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

test: restructure test suite #27

Closed
wants to merge 6 commits into from
Closed

Conversation

afuetterer
Copy link
Contributor

@afuetterer afuetterer commented Jan 8, 2024

This PR proposes the following changes:

  • move the existing tests to a tests directory (common practice)
  • split the single large test module in multiple modules per tested pydracor class
  • run tests in parallel

I would like to propose some more enhancements to the test suite. But this initial restructuring could be a starting point. What do you think?

@afuetterer afuetterer marked this pull request as draft January 8, 2024 15:34
@afuetterer afuetterer mentioned this pull request Jan 8, 2024
@afuetterer afuetterer marked this pull request as ready for review January 8, 2024 17:27
@afuetterer afuetterer force-pushed the tests branch 2 times, most recently from 3870711 to 81e8118 Compare January 9, 2024 12:02
@afuetterer
Copy link
Contributor Author

This is a first step to making the tests faster. 19 instead of 25 minutes. What do you think @hsluytergaethje?
The main reason for the slow tests is the constant accessing of the real API.

@cmil
Copy link
Member

cmil commented Jan 15, 2024

@afuetterer Thanks a lot for working on making the tests faster. But I think we should put this on hold until we solved a few architectural issues in pydracor. One of them being that the tests currently always run against the production system, which, given the high load they cause on the API, is not a good idea. We already scheduled a discussion of the issues for late January. Please keep the PR open and stay tuned.

@afuetterer
Copy link
Contributor Author

Are there any updates on this?

@cmil
Copy link
Member

cmil commented Apr 10, 2024

@afuetterer Can we rebase the tests branch to the current main of dracor-org/pydracor? @lehkost had made changes to mixnmatch.csv which created a conflict. We should make sure that these changes get properly merged.

We should then merge the PR since it reorganises the tests in a good way, but probably still disable the workflow or set it up so that it needs to be triggered manually. The tests still run 19min and put an unnecessary load on the production system. I wouldn't won't this to be executed automatically on every push.

We are still discussing how to restructure the library but a major overhaul looks very likely right now. So for the time being more work on the tests themselves is probably not a good idea.

@afuetterer
Copy link
Contributor Author

afuetterer commented Apr 10, 2024

Yes, sure.

I wanted to propose to "record" responses from the API using vcr.py, so the production system is only queried once when recording and not during testing (proposed in #24).

But sure, go ahead and restructure the library first.

I thought you had a discussion about that in January.

Do you want to follow up on #7?

@cmil
Copy link
Member

cmil commented May 5, 2024

@afuetterer Thanks for rebasing! Since the tests now fail I'm going to disable the work flow for now, though.

I wanted to propose to "record" responses from the API using vcr.py, so the production system is only queried once when recording and not during testing (proposed in #24).

I closed #24 because optimizing test on something that needs to be replaced seems like a waste of time.

I thought you had a discussion about that in January.

Do you want to follow up on #7?

We are planning to use some code generation framework for the next pydracor version. However, since nobody here is working full-time on it, it may take some time. Please stay tuned.

@afuetterer afuetterer closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants