Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test execution on Linux (*nix) #91

Closed
wants to merge 4 commits into from

Conversation

mpdeimos
Copy link

Currently test files are created using tmpdir + "dir" + randomValue.

On Linux (and most other *nix systems like Mac OS) tmpdir mostly yields /tmp (note no trailing slash) and above concatenation results in e.g. /tmpdir0123456. However, regularly you do not have access rights to write directly in the filesystem root and thus several of the tests fail. Writing in /tmp/dir0123456 would work, tho.

The following pull request contains this fix with two additions:

  • Changing from random folder numbers to a counter for the "number part" of the test directory. Doing so will prevent the tests from flickering.
  • I've added another directory layer, so all test files are in one directory (makes deleting easier)

This results in e.g. /tmp/st-tmp-dir/56on my machine.

Something else:
In my test environments I usually clear such temp directories in a create method, so I am sure no files from previous test runs are present. Should we add this here as well?

@mpdeimos mpdeimos changed the title Text execution on Linux (*nix) Test execution on Linux (*nix) Sep 24, 2014
@parrt
Copy link
Member

parrt commented Sep 24, 2014

Hi! I know this is a minor change, but could you add a signature to the contributors file and include it on this pull?

@sharwell
Copy link
Member

The inclusion of st-tmp-dir in the test path could be helpful for a number of reasons (including easy cleanup of outputs from failed tests), but changing the numbering seems problematic in the long run.

@mpdeimos
Copy link
Author

Will include the signature tomorrow.

What do you think is problematic about changing the numbering?
On Sep 24, 2014 8:38 PM, "Sam Harwell" notifications@github.com wrote:

The inclusion of st-tmp-dir in the test path could be helpful for a
number of reasons (including easy cleanup of outputs from failed tests),
but changing the numbering seems problematic in the long run.


Reply to this email directly or view it on GitHub
#91 (comment).

@sharwell
Copy link
Member

I like the idea of creating a new directory for each test, without worrying about the possibility of a previous test affecting the current test. I also like the ability to open the temporary directory and review outputs from tests that failed previous runs. The change to the numbering would defeat both of these and you have not described any benefits to the change.

Signed-off-by: Martin Pöhlmann <mpdeimos@users.noreply.github.com>
Signed-off-by: Martin Pöhlmann <mpdeimos@users.noreply.github.com>
Signed-off-by: Martin Pöhlmann <mpdeimos@users.noreply.github.com>
@mpdeimos
Copy link
Author

@parrt Signed off the commit and added the signature (PR updated)

@sharwell I think I got your point now, lemme try to explain. In general, I just stumbled across the random directory generation, while I tried to get the tests working on my linux machine. Using something unpredictable bears the risk that tests are flickering, e.g. in one testrun the same numbers are created twice. The odds are not high for this, but still.

I like the idea of creating a new directory for each test, without worrying about the possibility of a previous test affecting the current test.

"Previous test" is a ambiguous, I guess you mean "The last time this test method was executed (by the previous test run"?

True, therefore I would even prefer to empty the directory, as this keeps the test predictable and prevents side effects. The odds that a randomly generated temp dir will already be present from any previous run and thus get a clash will increase by every time you execute tests.

I also like the ability to open the temporary directory and review outputs from tests that failed previous runs.

That is true, with the current approach this is not possible for the tests using the random directory, but was tedious before as well, because you would have to keep the console output of ST from the previous run, grasp the random ID and then navigate to the dir.
Besides, there are plenty of tests just referring to tmpdir instead of the randomly (or sequentially) created ones and creating files in there. Here we can't even preserve the output within one test run. (I've added a commit to the pull request such that tmpdir already includes the st-tmp-dir suffix.)

But don't get me wrong, I have no pain changing this back to the random file generation.

@sharwell
Copy link
Member

How about using System.nanoTime() instead?

tmpdir + File.separator + System.nanoTime()

@mpdeimos
Copy link
Author

Would also be fine. Only problem is that test dirs keep accumulating.

@sharwell
Copy link
Member

That's fine (same as what it does now). You could either create a new issue to address it, or implement cleanup for passing tests like is implemented for ANTLR 4:

https://github.com/antlr/antlr4/blob/7f577d92090ccfd7e945040ef96da8d09d2211c7/tool/test/org/antlr/v4/test/BaseTest.java#L138-L147

@mpdeimos
Copy link
Author

Changed the behavior to use System.nanoTime().

Actually the @Rule stuff is how I manage temp dirs as well.

@sharwell
Copy link
Member

Hi @mpdeimos,

Sorry it took so long for us to come back to this. I created pull request #105 which provides the functionality described here, and also some additional functionality related to cleaning up the test outputs. Can you let me know if you have any trouble with it?

@sharwell sharwell closed this Feb 19, 2015
@mpdeimos
Copy link
Author

Hi, this works now on my machine with Linux. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants