8000 Travis does not run `make test-doc` when `pytest` fails · Issue #12237 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Travis does not run make test-doc when pytest fails #12237

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.

Sign up for GitHub

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

Jump to bottom
Closed
NicolasHug opened this issue Oct 1, 2018 · 9 comments · Fixed by #12248
Closed

Travis does not run make test-doc when pytest fails #12237

NicolasHug opened this issue Oct 1, 2018 · 9 comments · Fixed by #12248
Labels
Build / CI help wanted

Comments

@NicolasHug
Copy link
Member

The file that travis reads for testing is test_script.sh, and it is supposed to run pytest and then make test-doc

However, it seems that make test-doc isn't run if the first pytest command fails. This happened to me on #11705:

The issues in the second log could have been caught the first time, avoiding a reviewing round.

I have no idea why make test-doc isn't run though, looking at test_script.sh there's no reason to stop testing if the first pytest fails
@thomasjpfan
Copy link
Member

At the beginning of the bash script, it contains set -e. This is a shortcut for set -o errexit, which tells the bash script to exit immediately when a command has a nonzero exit code.

What is the desired behavior for test_script.sh? Do we want make test-doc and pytest to run independently of each other? If that is the case, we can adjust .travis.yml as follows:

script: 
- bash build_tools/travis/test_script.sh
- bash build_tools/travis/test_docs.sh

And add test_docs.sh script to just run make test-doc when "$SKIP_TESTS" != "true". The CI will fail when either script command fails.
8000

@thomasjpfan thomasjpfan mentioned this issue Oct 2, 2018
Merged
@jnothman
Copy link
Member
jnothman commented Oct 3, 2018 via email

@qinhanmin2014
Copy link
Member

I think I agree with Joel here. We usually get a long log from Travis and this will make users hard to find the errors. And @thomasjpfan has figured out that this is the expected behavior of the script.
Closing, thanks @NicolasHug for the issue and thanks @thomasjpfan for the PR.

@NicolasHug
Copy link
Member Author

The only problem is about frustration: when you fix all errors in the travis log, you expect it to go green the next time.

Also a passing make test-doc only adds about 50 lines at the end of the log so it's not that much compared to the 2K+ lines of a usual log.

@qinhanmin2014
Copy link
Member

Also a passing make test-doc only adds about 50 lines at the end of the log so it's not that much compared to the 2K+ lines of a usual log.

I guess we're talking about situations when normal tests and doc tests fail. Will it be difficult to locate the failures in normal tests then?

@NicolasHug
Copy link
Member Author

I don't think so, there would still be about 50 lines between the end of the failed normal tests and the beginning of failed test-doc.

@qinhanmin2014
Copy link
Member

I don't think so, there would still be about 50 lines between the end of the failed normal tests and the beginning of failed test-doc.

OK, I think I'm persuaded by you at this point. Do you have any response to Joel's comment : "And I think if both were Travis script entries, it would still stop at the first failure." (I guess Travis will continue?)

@qinhanmin2014 qinhanmin2014 reopened this Oct 3, 2018
@NicolasHug
Copy link
9AC2 Member Author

Do you have any response to Joel's comment : "And I think if both were Travis script entries, it would still stop at the first failure."

From Travis docs,

When one of the build commands returns a non-zero exit code, the Travis CI build runs the subsequent commands as well, and accumulates the build result

So I believe @thomasjpfan PR is a correct solution

@qinhanmin2014
Copy link
Member

So I believe @thomasjpfan PR is a correct solution

Fine, this is exactly what I've found. I'm persuaded by you (though I'll leave the PR to someone more familiar with Travis to ensure the correctness of it). ping @jnothman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Build / CI help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[MRG] Refactors doc test into seperate script thomasjpfan/scikit-learn
4 participants
@jnothman @NicolasHug @thomasjpfan @qinhanmin2014
0