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

Broken test for duplicate error codes #71

Closed
sbrugman opened this issue Sep 3, 2024 · 3 comments · Fixed by #73
Closed

Broken test for duplicate error codes #71

sbrugman opened this issue Sep 3, 2024 · 3 comments · Fixed by #73

Comments

@sbrugman
Copy link
Collaborator

sbrugman commented Sep 3, 2024

Issue

The test that checks whether there are any duplicate error codes never fails.
The TOR203 error code is defined three times and the test still passes:

https://github.com/pytorch-labs/torchfix/blob/main/torchfix/visitors/vision/singleton_import.py#L9

Root cause

The test itself checks if it has already seen the TorchError object, not the specific error code:

https://github.com/pytorch-labs/torchfix/blob/main/tests/test_torchfix.py#L68

Because the object is initialised three times, the objects are distinct.

Solution

The test should check on the error code and the TorchError should only be initialised once in singleton import.

As a fix I'd like to directly address #6, by improving the TorchError data structure. (Already got the code)

@kit1980
Copy link
Contributor

kit1980 commented Sep 3, 2024

TOR203 used three times in https://github.com/pytorch-labs/torchfix/blob/main/torchfix/visitors/vision/singleton_import.py#L9 is totally expected.

The test is to prevent different visitors to use the same code accidentally. Using the same code with slightly different messages in the same visitor is fine, otherwise there will be not enough error codes soon (and also some messages are dynamic).

@kit1980
Copy link
Contributor

kit1980 commented Sep 3, 2024

But the test itself seems to be broken indeed.

@kit1980
Copy link
Contributor

kit1980 commented Sep 3, 2024

Looks like the test was broken by #46

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 a pull request may close this issue.

2 participants