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

Update testing #14

Merged

Conversation

pseudo-rnd-thoughts
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented May 24, 2023

Personally, I found all of the testing confusing and unclear what was happening.
I have replicated the tests using pytest.mark.parametrize rather than pytest.fixture as this makes it more clear what is being iterated over IMO.

Looking at the current testing, 3341 passed, 4388 skipped, 13 warnings in 15.63s which is an alarming number skipped. I found the cause is conftest.py which is skipping the tests if the ROMs are missing. I have removed this file exposing the issue rather than hiding it.
The new pytest status is 3,369 passing, 0 skipped, 4,360 failing

DON'T MERGE UNTIL TESTS ARE PASSING

Changes

  • Remove conftest.py as this was a hack to skip tests
  • Removed from enum import Flag compatibility code as stable-retro doesn't support python 3.6 anymore so not an issue anymore
  • Moved all python testing into tests/test_python/ and cpp testing into tests/test_cpp/
  • Moved all retro.testing.__init__.py code to tests/test_python/__init__.py
  • Removed retro/testing/verify_changes.py as this appeared to be for travis.py which is now removed
  • Simplify retro/testing/__init__.py for game fixture to just an equivalent list all_games in tests/test_python/__init__.py
  • Remove handle function in favor of explicitly testing len(warnings) == 0 and len(errors) == 0

@victorsevero
Copy link
Collaborator

victorsevero commented Jun 5, 2023

Aren't the tests supposed to be skipped? Since most ROMs can't be tested (we couldn't use illegal ROMs), we don't want them to throw errors, but we want to make sure that anyone that uses their own legal copy of the ROM is able to test them before committing a new game environment.

@pseudo-rnd-thoughts
Copy link
Member Author

@victorsevero Yes, this is what I did in deeac25

The reminding error is

FAILED tests/test_python/test_integration.py::test_missing - AssertionError: assert 4 == 0
 +  where 4 = len([('Bomberman-Nes', 'data.json'), ('Bomberman-Nes', 'scenario.json'), ('Bomberman-Nes', 'metadata.json'), ('IceHockey-Nes', 'scenario.json')])

It looks like Bomberman and IceHockey are new environment (not part of the original project). @MatPoliquin What should we do with them?

Currently, I am just logging the warnings, @MatPoliquin do you understand what these mean or why?

tests/test_python/test_integration.py::test_data[BattleCity-Nes-]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:12: UserWarning: WARN: BattleCity-Nes/data.json: suspicious type >u2 for lives

tests/test_python/test_integration.py::test_data[BoxingLegendsOfTheRing-Genesis-]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:12: UserWarning: WARN: BoxingLegendsOfTheRing-Genesis/data.json: suspicious type |u1 for score

tests/test_python/test_integration.py::test_data[BoxingLegendsOfTheRing-Genesis-experimental]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:12: UserWarning: WARN: experimental/BoxingLegendsOfTheRing-Genesis/data.json: suspicious type |u1 for score

tests/test_python/test_integration.py::test_data[BoxingLegendsOfTheRing-Genesis-all]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:12: UserWarning: WARN: all/BoxingLegendsOfTheRing-Genesis/data.json: suspicious type |u1 for score

tests/test_python/test_integration.py::test_data[BrawlBrothers-Snes-]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:12: UserWarning: WARN: BrawlBrothers-Snes/data.json: suspicious type |d1 for score

tests/test_python/test_integration.py::test_data[HangOn-Sms-]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:12: UserWarning: WARN: HangOn-Sms/data.json: suspicious type >d5 for score

tests/test_python/test_integration.py::test_scenario[DavidCranesAmazingTennis-Genesis-experimental]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:20: UserWarning: WARN: experimental/DavidCranesAmazingTennis-Genesis/scenario.json: suspicious done condition any with more than 2 checks

tests/test_python/test_integration.py::test_scenario[ForemanForReal-Genesis-]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:20: UserWarning: WARN: ForemanForReal-Genesis/scenario.json: suspicious done condition any with more than 2 checks

tests/test_python/test_integration.py::test_scenario[VirtuaFighter2-Genesis-]
  /home/runner/work/stable-retro/stable-retro/tests/test_python/test_integration.py:20: UserWarning: WARN: VirtuaFighter2-Genesis/scenario.json: suspicious done condition any with more than 2 checks

@zbeucler2018
Copy link
Collaborator

@pseudo-rnd-thoughts I am unsure about the error but I think I can help with the warnings.

Each game in stable-retro has some mappings for certain variables in the ROM's RAM. Each game needs a data.json file which tells stable-retro what address it should look at for a specific variable, and how to format the data from that address. The variables defined in a game's data.json are the variables in the info dict returned from env.step().

For example, the BattleCity-Nes game has a variable lives which is at address 58 and should be interpreted as 2 big endian unsigned bytes (aka >u2) . Heres the page on the original documentation that talks more about it.

The lines that are throwing these errors are stable-retro/retro/testing/tools.py lines 94-96.

I believe that these warnings are just to ensure that the original integrated games had the correct byte-format-to-address mapping for the important variables lives and score.

Maybe they were assuming that in most of the games, the lives variable is usually formatted as a single unsigned byte (|u1) , a single signed byte (|i1) , or a single binary-coded decimal byte (|d1)

@MatPoliquin
Copy link
Collaborator

@pseudo-rnd-thoughts For Bomberman and Icehockey we can remove them for now until I properly integrate them.
For the warning, adding to zbeucler2018's comments, you can ignore them for now, they are likely false flags. For example HangOn-Sms which I integrated myself works well (A model can beat the whole game) and data is read in the correct format.

@MatPoliquin MatPoliquin merged commit 968894b into Farama-Foundation:master Jun 13, 2023
6 checks passed
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

4 participants