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

Cleaning up grpauto.tst and other tests #964

Closed
4 tasks done
olexandr-konovalov opened this issue Nov 28, 2016 · 7 comments
Closed
4 tasks done

Cleaning up grpauto.tst and other tests #964

olexandr-konovalov opened this issue Nov 28, 2016 · 7 comments
Labels
topic: tests issues or PRs related to tests
Milestone

Comments

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Nov 28, 2016

Nightly tests in the master branch are failing after #896. I am creating new issue to continue discussion here instead of commenting on a merged PR:

@hungaborhorvath
Copy link
Contributor

In #763 I manually changed the InfoLevel of InfoWarning in the test file to make sure to not get this warning message. It was two extra lines and it took care of the Alternating recognition needed message.

I am wondering, though, would it be possible to have an automatic and visible test for without packages for a PR? Then one could see immediately if some of the code breaks tests without the packages. Before I never even thought about checking my code without packages, because I thought travis is taking care of all tests. (And btw, why exactly do we test for without packages, if GAP is shipped with some default packages, anyway?)

@olexandr-konovalov
Copy link
Member Author

@hungaborhorvath : I've looked at the line which prints Alternating recognition needed and I can't see a reason why it uses InfoWarning while the rest of the info messages in this file are using InfoLattice, so I've changes it to InfoLattice to in 488af13 - now testinstall should pass.

Testing GAP with/without packages is important due to complicated dependencies and not all packages working everywhere. There are no fundamental obstacles to do it as you suggest on Travis - the limitation of Travis tests is due to its restricted resources, and our willing for a faster turnover: you will have to wait twice longer for the test to complete if you will run each configuration with/without packages on Travis...

@olexandr-konovalov
Copy link
Member Author

@hungaborhorvath P.S. and of course we need a way to check that packages does not break the behaviour of the core system, so we need a reference run of the tests without packages.

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Nov 28, 2016
@olexandr-konovalov olexandr-konovalov added the topic: tests issues or PRs related to tests label Nov 28, 2016
@olexandr-konovalov
Copy link
Member Author

I have removed some more heavy tests in cf1c2fb, dcaa4b2 - now grpauto.tst is runnable, and it takes about 2000 seconds both with and without packages.

This means that we're able to merge and test further PRs, and I am going to merge #763 by @hungaborhorvath now.

However, the "alternating recognition message" is still there when GAP is started with -A and I would appreciate if @hulpke could say whether it's OK to show it with InfoPerformance level 2.

I've removed the dependency of this issue on benchmarks, as those may be independently continued in #966.

@hulpke
Copy link
Contributor

hulpke commented Dec 1, 2016

I'm not fighting for the level of InfoPerformance, but the whole idea of this InfoClass was to warn if something carelessly inefficient is happening. I find it somewhat strange that we are on the path of having the test files dictate this user interface.

My recommendation would be to rather have the tests deliberately turn InfoPerformance (maybe even InfoWarning) to zero to ensure they do have to bother with such information.

@olexandr-konovalov
Copy link
Member Author

@hulpke - we have mechanism for that: I suggested already to use START_TEST and STOP_TEST to play with InfoLevel elsewhere, but you haven't commented on that. For now, only InfoPerformance will suffice.

@olexandr-konovalov
Copy link
Member Author

InfoPerformance now handled in 392ad82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: tests issues or PRs related to tests
Projects
None yet
Development

No branches or pull requests

3 participants