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

pythonPackages.bjoern: fix test result checking #62055

Closed
wants to merge 1 commit into from

Conversation

risicle
Copy link
Contributor

@risicle risicle commented May 25, 2019

Motivation for this change

When testing #61887 noticed test_wsgi_compliance.py returns a zero exit status unconditionally - to check for a failure we need to grep the stdout. It also appears this test is broken upstream for py27 - I should open an issue with them...

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

test_wsgi_compliance.py returns a zero exit status unconditionally - to
check for a failure we need to grep the stdout. it also appears this test
is broken upstream for py27.
@@ -13,7 +13,8 @@ buildPythonPackage rec {

checkPhase = ''
${python.interpreter} tests/keep-alive-behaviour.py 2>/dev/null
${python.interpreter} tests/test_wsgi_compliance.py
'' + stdenv.lib.optionalString isPy3k ''
${python.interpreter} tests/test_wsgi_compliance.py | (! grep -i "fail")
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, this should be handled upstream. Currently it just print the message https://github.com/jonashaag/bjoern/blob/3.0.0/tests/test_wsgi_compliance.py#L39-L45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely (upstream issue jonashaag/bjoern#158), but in the meantime the way we have the tests setup now, something breaking test_wsgi_compliance.py in a more serious way won't get picked up at all.

@worldofpeace
Copy link
Contributor

Upstream issue has been closed, so maybe just add jonashaag/bjoern@eaca4b5 as a patch here.

We then can probably merge #61887

@risicle
Copy link
Contributor Author

risicle commented Jun 1, 2019

Hmm no such luck I'm afraid - applying that patch to either 2.2.3 or 3.0.0 causes the py37 tests to fail. (of course we could always conditionally apply the patch :)

@risicle
Copy link
Contributor Author

risicle commented Jun 1, 2019

See jonashaag/bjoern#160

@risicle risicle mentioned this pull request Jun 7, 2019
10 tasks
@risicle risicle closed this Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants