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

Gives warning when encountering free range .enc files in test directory #464

Merged
merged 6 commits into from
May 31, 2016
Merged

Gives warning when encountering free range .enc files in test directory #464

merged 6 commits into from
May 31, 2016

Conversation

supercooldave
Copy link

The existing test script silently passed over .enc files in the test directories that were not set up as proper tests or ignored in the DISABLED_TESTS.grep file. This patch changes the test script to give a warning when encountering those files.

@kaeluka
Copy link
Contributor

kaeluka commented May 30, 2016

I'll check this out in the afternoon.

@kaeluka
Copy link
Contributor

kaeluka commented May 31, 2016

This does what it says, but it adds so much output to some test suites that I'd rather not have this feature. The reason is that so many files are not tests by design. There should at least be a way to disable the warning for files that are supposed to be no test.

Example:

>>> RUNNING TEST_SUITE ./encore/modules
    Found Makefile in ./encore/modules. Running!
make[3]: *** No rule to make target `clean'.  Stop.
           make | make[3]: Nothing to be done for `build'.
 - CallImported...
 - ClosureImp...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - ClosureImporter...
   ... disabled in src/tests/DISABLED_TESTS.grep
 - DeepImporter...
 - DeepLib...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - DupImporter...
 - FarImporter...
 - Importer...
 - Lib...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - List...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - MidImport...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - PassiveImport...
 - Qualified...
 - StdLib...
 - Utils...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - Vector...
   ... warning: .enc file present but missing .out, .fail or .chk file.
 - VectorTest...
 - trait_import...
 - trait_import_module...
   ... warning: .enc file present but missing .out, .fail or .chk file.

@kaeluka
Copy link
Contributor

kaeluka commented May 31, 2016

Would it be an option to add an empty .out file for these? I'm guessing that they can't have a Main class, right? (and therefore can't run)

@supercooldave
Copy link
Author

The point of the feature is to force the tester to clean up the directory, for instance by putting the relevant file names in the DISABLED_TESTS.grep file. The point is not to disable the warnings, but to indicate which files are not tests.

If anything, the warnings about tests occurring in the DISABLED_TEST.grep file should be disabled.

I do not like the default empty .out file idea. It is too error prone.

@supercooldave
Copy link
Author

I should point out that this feature was present in your original test suite.

@kaeluka
Copy link
Contributor

kaeluka commented May 31, 2016

Adding it to DISABLED_TESTS.grep would be much better than the empty .out file (which I do not like either). However, I tried this out, and it does work, but now a different warning appears: disabled in src/tests/DISABLED_TESTS.grep. If we go this way, we should disable the warning about a test being being skipped. I'm worried that we have lots of warnings constantly and people start to not read them.

I'd be happy to merge if:

  • the warning message would be amended to suggest adding the test to DISABLED_TESTS.grep,
  • we don't warn about disabled tests any more.

If we want to keep an overview of how many tests are warned, we might consider adding an extra counter for number of skipped tests per test suite in a separate PR.

@kaeluka
Copy link
Contributor

kaeluka commented May 31, 2016

I'd like to point out that going through DISABLED_TESTS.grep is a bit hacky: a file without a specification is not a test in the first place. We shouldn't need to disable a test that is not a test. But it seems pragmatic to not over engineer and reuse the infrastructure we already have.

@supercooldave
Copy link
Author

I can make the two suggested changes.

@kaeluka
Copy link
Contributor

kaeluka commented May 31, 2016

And I'll watch this space!

@supercooldave
Copy link
Author

If DISABLED_TESTS.grep was called something different, like .gitingore but not, then there would be no semantic problem.

@kaeluka
Copy link
Contributor

kaeluka commented May 31, 2016

If DISABLED_TESTS.grep was called something different, like .gitingore but not, then there would be no semantic problem.

That's true.

@supercooldave
Copy link
Author

I've addressed the issues mentioned above and added support for .flags, as previously discussed.

cd ${DIR}
${ENCOREC} ${FLAGS} ${NAME}.enc
echo "HEREHERE ${FLAGS}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to remove these echo calls :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can actually do that

@supercooldave
Copy link
Author

Thanks.

Stephan Brandauer added 2 commits May 31, 2016 16:41
@kaeluka kaeluka merged commit 45a8325 into parapluu:development May 31, 2016
@supercooldave supercooldave deleted the fix/tests branch June 20, 2016 12:46
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.

3 participants