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

Fix 458 - Test system updates #463

Merged
merged 13 commits into from
May 27, 2016
Merged

Fix 458 - Test system updates #463

merged 13 commits into from
May 27, 2016

Conversation

kaeluka
Copy link
Contributor

@kaeluka kaeluka commented May 27, 2016

This is the response to #458

  • calls module system three times -- FIXED
  • remove Makefile in typesyn dir -- FIXED
  • empty .fail spec makes test pass -- FIXED
  • smaller UI glitches -- FIXED
  • can not run individual tests -- FIXED (f.ex bin/test encore/basic/activeThis)
  • gracefully handle rubbish input. -- FIXED
  • test outputs some rm message -- FIXED

Additionally:

  • removed all but one Makefiles

@supercooldave
Copy link

supercooldave commented May 27, 2016

It's not so clear which issues were not fixed.
Why should I do the work to decode this information when you could simply provide it?

@supercooldave
Copy link

Also, title of the PR will not leave any useful information in the CHANGELOG.md file.

@kaeluka kaeluka changed the title Fix 458 Fix 458 - Test system updates May 27, 2016
@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

  • Does not support multiple DISABLED_TESTS.grep files

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

Why should I do the work to decode this information when you could simply provide it?

You're misunderstanding. I sent a PR. I didn't assign you.

@supercooldave
Copy link

Why should I do the work to decode this information when you could simply provide it?
You're misunderstanding. I sent a PR. I didn't assign you.

I was suggesting that your comment would have been better to say which aspects of #458 were addressed and which ones were not addressed. Since I need to create another issue regarding the ones not addressed, it would be useful to know which ones they are (rather than having to deduce from the clues left behind.)

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

I'm aware of the one I mentioned above. But only you can have a final say on whether the list is complete -- because you provided most of the input. I might have overlooked something.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

Does not support multiple DISABLED_TESTS.grep files

that's the one.

@supercooldave
Copy link

And the command line arguments .flags file one.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

IMHO, that's addressed pretty well. We only have one Makefile left, and it's tiny. But further discussion of this should be on the issue that you might create about it.

@supercooldave
Copy link

Whether it has been addressed or not, none of the comments above indicate that.

@supercooldave
Copy link

Anyway, I'm looking at the PR now.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

Amended the PR text.

@supercooldave
Copy link

This point:

when the script encounters a .enc file without a corresponding .out file, it simply prints the name of the file, and continues. It does not run treat it as a test, which is fine, but it does not say what it has done with the file. The output is exactly the same as for a test that runs and succeeds.

was addressed by completely ignoring the offending files. I would suggest putting a warning to say that a .enc file was encountered but no accompanying .fail etc was found, is more informative, and could help the tester avoid unwanted errors and strange behaviour -- "Why can't the test script see my file???"

@supercooldave
Copy link

The output still says

- Importer...
rm: encore/modules/Lib: is a directory

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

  • Importer...
    rm: encore/modules/Lib: is a directory

I couldn't reproduce this in the end, so I assumed it had been addressed. Is this directory a temporary one?

@supercooldave
Copy link

encore/modules/Lib is an incorrectly named directory in the DISABLED_TESTS file. It should be lowercase lib.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

fixed!

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

I'll look at the rm thing.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

The error message should be gone now!

@supercooldave
Copy link

Just the comment #463 (comment) above to go.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

I have to put this task on the back burner for a bit. It'd be better to make an issue about this and deal with it later.

@supercooldave
Copy link

I'll merge and try to fix it then.

@supercooldave supercooldave merged commit 285706b into parapluu:development May 27, 2016
@kaeluka
Copy link
Contributor Author

kaeluka commented May 27, 2016

Great, thank you!

@kaeluka kaeluka deleted the fix-458 branch May 27, 2016 14:54
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