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

Tests duplicated deleted from parametrized_envs #1074

Merged
merged 7 commits into from
Aug 28, 2023

Conversation

GiovanniGrotto
Copy link
Contributor

@GiovanniGrotto GiovanniGrotto commented Aug 23, 2023

Description

I deleted all the repeted lines in parametrized_envs from all_parameters_combs_tests.py, and changed lines where the kwargs where matching with default arguments with an empty dict, so now the test will run with default args also if they will be changes in init func.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

This is an exmaple of what I changed, the first highlighted line now has an empty dict instead of hand_size=5 and the second and third lines have been deleted.
Immagine 2023-08-19 153353

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@elliottower elliottower left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for taking the time to fix this. Curious what the speedup will be in the CI. If you could also try to cross reference with the pytest runner file and see if we actually need both of these files (my guess is that was created first and this is the more comprehensive version of it, you could check git blame to figure out when each was created and stuff like that as well)

@GiovanniGrotto
Copy link
Contributor Author

Looks good to me, thanks for taking the time to fix this. Curious what the speedup will be in the CI. If you could also try to cross reference with the pytest runner file and see if we actually need both of these files (my guess is that was created first and this is the more comprehensive version of it, you could check git blame to figure out when each was created and stuff like that as well)

Yes I'll check in the next days an try to speed up test as much as possible, I can also check Gymnasium wrapper for max_cycles

…ement so the screen gets properly rendered every time
1.) Added missing env so now run test for every env
2.)Added check that was previously run only by pytest_runner_test
In this way now all_parameter_combs_test.py is combining the old version of itself with pytest_runner_test avoiding running repeated test
@elliottower
Copy link
Member

Looks good, only thing I'd say is try to at least do two parameter combinations per environment (I imagine the test will take about the same amount of time if you just remove the pytest runner test and do this, because most envs are already being run twice with the two files)

1.) Added max_cycles to all env that accept the parameter
2.) Added a check with parameters different from default ones for env that accept them
@elliottower
Copy link
Member

Good stuff, the tests are now 15 mins instead of 30 mins

@elliottower elliottower merged commit 1742c8d into Farama-Foundation:master Aug 28, 2023
33 checks passed
elliottower added a commit that referenced this pull request Aug 28, 2023
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.

None yet

2 participants