Skip to content

Unit test enhancement #523

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Mahmoud-Khawaja
Copy link
Contributor

now testProxyCreationInCaseOfChoiceGenerator() is faster:

in the original approach we were creating the thread then the proxy in the main thread. but now we are creating the proxy in the main thread then the thread itself. so this can reduce conncurrent operation and minimize the satate space.

and when it comes to verification order the original approach verifying the the cross thread equality then the class format now i switched it as it can front-loads complex verification in single-threaded contex.

the old test were taking up to 14 seconds now its about ~1 second

@Mahmoud-Khawaja
Copy link
Contributor Author

Hello @cyrille-artho,
i don't why change_clock_duration_test fails here i haven't done any changes for this one. can you please help?

Copy link
Member

@cyrille-artho cyrille-artho left a comment

Choose a reason for hiding this comment

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

Good work; in one case, you have probably optimized too much. Try to see if you can keep some interleavings in ProxyTest; if the only way is to keep all interleavings, then we just revert the change I commented on (and keep the test as slow as it is now).

NewThread t = new NewThread();
t.start();
MyHandler handler = new MyHandler(42);
Copy link
Member

Choose a reason for hiding this comment

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

If you move all code of the current thread up to before the new thread is created, there is nothing left to interleave for JPF. It may be the case that this test cannot be optimized without losing the fact that it verifies thread interleavings here.

@cyrille-artho
Copy link
Member

Very few tests fail occasionally, but luckily, only rarely. I have restarted the CI build, and while waiting found that you probably removed the thread interleaving that the unit test in ProxyTest wanted to have.

@Mahmoud-Khawaja
Copy link
Contributor Author

i see, this one is hard to optimized as it will risk compromising the thread interleaving verification. However, i think the most optimization we can do is to pre compute the expected class name it can reduce the time but i don't think it will be a significant imrpovement. it can be something like ~3-4 seconds. i will try to do my best here to see if further significant improvements can be done

Thanks a lot for your help!

@cyrille-artho
Copy link
Member

Sometimes the required complexity of a test is intrinsic (it's in the nature of the problem) and cannot be reduced further. In that case, that's fine, and at least we have reviewed the test now and know the complexity is not accidental (being caused by a poor approach to the problem).

@syheliel
Copy link
Contributor

Hello @cyrille-artho, i don't why change_clock_duration_test fails here i haven't done any changes for this one. can you please help?

Hi, I find this case fails in my machine also 🤣

@cyrille-artho
Copy link
Member

Does that test fail consistently on your machine, or occasionally? In the CI setup on GitHub, it fails only very rarely so we can keep the test. If it fails consistently on some systems, it is worth investigating this. If we can't fix the problem, we might remove the test instead.

@Mahmoud-Khawaja
Copy link
Contributor Author

It works on my machine no problem with it.

@syheliel
Copy link
Contributor

I'm using ubuntu 24 under vmware ESxi. At first, the test will fail no matter how many times I try. But now it works again(trying to repeat running for 200 times)... I think it may related to some time sync problem for VM and currently no need to fix

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