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

JSON files for test_class_optimize.py #116

Closed
wants to merge 0 commits into from

Conversation

NormannK
Copy link
Collaborator

@NormannK NormannK commented Oct 6, 2024

No description provided.

Copy link
Collaborator

@drbacke drbacke left a comment

Choose a reason for hiding this comment

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

Please stop to deleting my changes. This should be no standard test for now, it's way to expensive. Maybe we should reserve this for Releases e.g.

@pytest.mark.skip(reason="Expensive - Skipped per default")

@NormannK
Copy link
Collaborator Author

NormannK commented Oct 6, 2024

sry that was not my intention. As i suggested yesterday, there ware ways to only run that test if there are changes to the class file. But for now that will do.

@NormannK NormannK marked this pull request as draft October 6, 2024 16:34
@drbacke
Copy link
Collaborator

drbacke commented Oct 7, 2024

@NormannK No problem. I find the test extremely helpful and we can use it to identify most errors. So thanks about that :-)
How about we just run the optimization for this test for a few iterations?
10 would be enough to do a “rough” test. In this case, we can run that test in our standard test action

@NormannK
Copy link
Collaborator Author

NormannK commented Oct 7, 2024

How about we just run the optimization for this test for a few iterations? 10 would be enough to do a “rough” test. In this case,

I thought about that too. Even a single pass would do. I suggest to do it in two stages. #1 Pass for every day (PR) usage with only a single pass and the full pass we do manually as soon as there are changes to the file. As long as the maintainer know that we need this on file changes.

But the other problem is that this PR is not working. The json files are not found and i don't know why. all paths are correct. afaik.
And those json files would be also the data for the other tests which will need the same.

@michaelosthege
Copy link
Collaborator

@NormannK try to print DIR_TESTDATA and also the file you're trying to open.

I didn't spot anything from a quick look, but won't have time to take a closer look until tonight.

@NormannK
Copy link
Collaborator Author

NormannK commented Oct 7, 2024

@michaelosthege yeah i did that including the system path, setting the path manually and doing anything i know what to check. My last resort was it to upload here to see if its a problem on my end. Sadly its not.

@michaelosthege
Copy link
Collaborator

@NormannK I fixed the tests. I can't push your main branch, so I will open a fresh PR with a cleaned-up branch that also contains your changes.

@NormannK
Copy link
Collaborator Author

NormannK commented Oct 7, 2024

thank you very much.

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.

3 participants