-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add an option to reset the trackers #316
base: master
Are you sure you want to change the base?
Conversation
can i get reasons for this option? |
This is mainly for libp2p tests (but there may be other applications). When a tracker fails at the beginning of the tests, all other tests checking that tracker fail as well even if there's no problem. This is just an option to reset and improve readability. |
Rather than just resetting the trackers, it seems it would be useful in general to reset all global state that chronos creates - or at least as much as possible - that includes the global dispatcher etc - resetting just the trackers will merely lead to an inconsistency between the various globals that are spread out throughout the library (and indeed make testing harder) |
If this tracker check fails it means that you are leaking or do not properly cleanup in test. All the trackers working like that, i dont see any reasons to fix |
We know that the tracker is leaking, the point is to reset the state before other tests, otherwise all the following tests will fail. Ideally we would indeed reset all the global states (open sockets etc), but that requires tracking everything in chronos to be able to reset everything |
I don't know where I said I wanted to fix chronos, I said that I wanted to improve readability when a tracker shows some leaks. Currently we have logs like this one that are hard to read (and wrong because there's only one test that fails). This is simply a QoL feature. |
How tracker could leak? It just counters. From my point of view if you want to run group of tests you can bundle it into |
this is what reset in this PR is for: so that each test has a fresh counter value - otherwise, correct tests are shown as failing because a previous test failed. |
Also i think you don't understand that if you reset counters it doesn't matter that some loops which are pending and running are not running anymore. Counters got decreased when this loops exiting. So even in tests if you just reset counters you can get pretty weird behavior when some of the loops which are still running could produce some undefined behavior for your tests. |
thinking a bit more about this PR, without resetting the dispatcher I think it makes little sense to reset the tracker alone - ie the only way to get a "clean slate" for every test would be to run each test in a separate process (which implicitly will reset the tracker) - such functionality could quite easily be added to |
Also see #326, which opens the question "should closing the dispatcher close the associated FDs" (which could have been moved elsewhere) |
No description provided.