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

Fixes#16484: rl fix failed tests #2724

Conversation

gpoblon
Copy link
Contributor

@gpoblon gpoblon commented Jan 17, 2020

Fixed failing tests, and cleaned integration tests so compile.rs stays clean of code logic. Added tests comments to highlight that some tests could be meant to fail : uncomment to ensure tests are still working the way they are intended to.

https://issues.rudder.io/issues/16575

@amousset
Copy link
Member

Added tests comments to highlight that some tests could be meant to fail : uncomment to ensure tests are still working the way they are intended to.

Should these tests always fail or is it a temporary situation? We should generally not require manual modification of the code to run all tests. To ensure proper failure modes, the test logic should be reversed, and return a success if the tested code fails as expected (a bit like the #[should_panic] annotation that allows ensuring a code path panics in a test).

@gpoblon
Copy link
Contributor Author

gpoblon commented Jan 17, 2020

These tests were mainly here to show you how tests are handled :
There are 2 different kinds of errors in the test :

  • Result<_, Error> from the compile function. These are handled just as you say: if a file is prefixed by f_ and rudderc::compile() returns an error, then the test assertion will be Ok. Not Error.
  • Errors from the test feature itself, ie: something went wrong doing a test (name of the test file, error when writing the file etc) so it HAS to fail in order to notify the user something is wrong with tests themselves.
    The commented tests fall into the latter kind. If you try to test a file name that has a bad format (ie not prefixed by s_ or f_) the test itself is supposed to fail.
    So they have no vocation to be uncommented unless you want to be sure that the integration test feature works as intended.

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 648a0c1 into Normation:branches/rudder/6.0 Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants