Setup reporting for missing CSS file #390
Conversation
|
@gnikonorov I have only changed the way of reporting the missing CSS files. I can't see any new test cases for this change. If any, Please let me know. |
In the run method you can pass a variable list of arguments via |
|
Thanks @gnikonorov ! |
@gnikonorov Done. |
There was a problem hiding this comment.
Thanks for this @prakhargurunani !
I'm satisfied but I'll let @gnikonorov have the last word (and/or merge). 👍
testing/test_pytest_html.py
Outdated
| assert "No such file or directory: 'style.css'" in result.stderr.str() | ||
| assert "Missing CSS file: style.css" in result.stderr.str() | ||
|
|
||
| def test_multiple_css_files(self, testdir, recwarn): |
There was a problem hiding this comment.
Can you add a test for where only part of the passed files are missing? You can use https://github.com/pytest-dev/pytest-html/blob/master/testing/test_pytest_html.py#L321 as an example for creating adhoc files in tests.
Also, please use @pytest.mark.parametrize to combine your tests with the test above. It'll be a lot cleaner than having 2-3 separate tests for the same feature ( reporting of missing css files )
Here is an example of how we parametrize in this file
There was a problem hiding this comment.
@gnikonorov See 8ccc104
Have I used @pytest.mark.parametrize correctly ?
There was a problem hiding this comment.
Almost @prakhargurunani! Please see my last review
testing/test_pytest_html.py
Outdated
| assert len(recwarn) == 0 | ||
| assert expected in result.stderr.str() | ||
|
|
||
| def test_some_css_files(self, testdir, recwarn): |
There was a problem hiding this comment.
You can merge this test with the test above, just create the valid css file in the body of the test.
testing/test_pytest_html.py
Outdated
| ("abc.css", "xyz.css", "Missing CSS files: abc.css, xyz.css"), | ||
| ], | ||
| ) | ||
| def test_css_invalid(self, testdir, recwarn, file1, file2, expected): |
There was a problem hiding this comment.
This is correct, but you could make it even more clean by parametrizing with a list of files instead of passing file1 and file2. For example:
$ cat test_parametrize.py
import pytest
@pytest.mark.parametrize("arg", ["arg1", "arg2", ["arg3.a", "arg3.b"]])
def test_list(arg):
assert arg is not None
$
In the example above the test is run 3 times. First with "arg1", then "arg2", and finally with the list ["arg3.a", "arg3.b"]
This way your test can be expanded on in the future without changing its signature every time, and it becomes easier to reason about
|
I'll take a look tonight @prakhargurunani! Sorry, I've been busy |
testing/test_pytest_html.py
Outdated
| ], | ||
| ) | ||
| def test_css_invalid(self, testdir, recwarn, files): | ||
| assert files is not None |
There was a problem hiding this comment.
Is this debug code? files should never be None based on your parametrization
testing/test_pytest_html.py
Outdated
| assert result.ret | ||
| assert len(recwarn) == 0 | ||
| assert "No such file or directory: 'style.css'" in result.stderr.str() | ||
| if isinstance(files, list): | ||
| assert files[0], files[1] in result.stderr.str() |
There was a problem hiding this comment.
Sorry I missed this, but this doesn't do what you expect. You aren't actually checking for files[0]. This just checks whether or not files[0] is truthy and executes files[1] in result.stderr.str() if it's not. For example:
>>> l = [1,2,3]
>>> assert 0, 2 in l
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AssertionError: True
>>> assert 44, 2 in l
There was a problem hiding this comment.
LGTM, thanks @prakhargurunani !
I'll leave the final review/merge up to @BeyondEvil
|
@gnikonorov docs are failing, not sure what to do here? 🤔 |
FirePing32
dismissed stale reviews from BeyondEvil and gnikonorov
via
4086b9e
FirePing32
force-pushed
the
css-file-reporting
branch
from
d077d83 to
4086b9e
Compare
|
@BeyondEvil docs are failing because the RTD build was enabled before the PR for RTD was merged. Once #402 goes in the issue will go away |
fixes: #388
Returns missing CSS files in
OSErroraccording to the order they were passed.