search

pyvec / elsa

Helper module for hosting Frozen-Flask based websites on GitHub pages
Other
28 stars 49 forks source link

Use pytest.raises instead of xfail #19

Closed encukou closed 7 years ago

encukou commented 7 years ago

Expected failures are used for tests that should ideally not fail, but currently do. There should be an open bug for each xfail. For cases where the code should raise an exception, there's pytest.raises.

Sorry I missed this in the review.

hroncok commented 7 years ago

Shouldn't we make the exception common in both cases for easier readability?

encukou commented 7 years ago

Ideally yes, but the CalledProcessError constructor is currently undocumented, so (technically) only subprocess code is allowed to raise it. And I don't think switching to AssertionError would help readability.

hroncok commented 7 years ago

I mean't creating our own CommandFailed.

encukou commented 7 years ago

Not sure if that helps readability. Feel free to leave the second commit out.

hroncok commented 7 years ago

Well the machinery is less readable, but the tests are more readable. So I'd rather keep it.