Open momchil-flex opened 2 months ago
yaugenst-flex
commented
2 months ago There definitely are some tests that just take longer but that still need to be tested. What we could do is mark some tests as slow (pytest.mark.slow), and then we can decide when those should be run?
At some point we should maybe also think about refactoring the tests...
daquinteroflex
commented
2 months ago Actually that's strange. I named that file import_tidy3d.py so that it would not get caught in the main pytest command. Possibly, when the docstring tests were upgraded this got included in the main test suite accidentally. The goal was to have that file whenever we wanted to benchmark manually but shouldn't be commonly run. Possibly this involves just updating the testing configuration settings to ignore that file and that'd work?
I'm checking now how to avoid it being included, but running into a weird __pycache__ local testing issue. Will comment again or PR when I work out how to remove it properly.
yaugenst-flex
commented
2 months ago Actually that's strange. I named that file import_tidy3d.py so that it would not get caught in the main pytest command.
In our pyproject.toml, we set
[tool.pytest.ini_options]
python_files = "*.py"Meaning, we are globbing for any python file, and the test is then discovered due to it being named test_.
However, if this import thing is not actually a test but a script, maybe we should not put it under tests/?
daquinteroflex
commented
2 months ago Oh yeah, that was a doctest change. Nice, we should move it to scripts/ then. I can include this in the docs PR I'm trying to finish polishing atm.
yaugenst-flex
commented
2 months ago Alternatively, we could mark it, by registering a marker in pytest.ini:
[pytest]
markers =
optional: mark a test as optional.Mark the test:
@pytest.mark.optional
def test_import():
...And then we can exclude this marker from running by default and run it via pytest -m optional
We can do the same for slow tests.
momchil-flex
commented
2 months ago Oh yeah, that was a doctest change. Nice, we should move it to
scripts/then. I can include this in the docs PR I'm trying to finish polishing atm.
Maybe let's just have a tiny PR for that so it already saves ~3mins for the tests? :)
daquinteroflex
commented
2 months ago PR opened with the move!
daquinteroflex
commented
2 months ago To add to this issue.
Yannick suggested also to improve what we're caching in the Github actions. I think we can cache the poetry virtual environment but this did cause issues beforehand during the first poetry implementation, but I think can be revisited.
momchil-flex
commented
2 months ago I can leave this open for future improvements, but I think nothing else is planned for now?
Our tests are starting to get a bit heavy to run, so I timed them. Here's all that take more than 1s.
The
tidy3d_import.pyone imports 100 times, which seems excessive. It also doesn't really test anything, it only prints. Was it not supposed to be part of the test suite at all @daquintero, rather just for internal use?I made a PR for the
test_num_mediumsone.Apart from that there's a lot of adjoint/autograd stuff @tylerflex @yaugenst-flex , maybe some of it is inevitable, but maybe some can be improved? Not high priority.
Then there's some simulation data and mode solver ones, but again, not high priority.