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

Add an option to reset the trackers #316

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add an option to reset the trackers #316

wants to merge 1 commit into from

Conversation

lchenut
Copy link

@lchenut lchenut commented Sep 22, 2022

No description provided.

@cheatfate
Copy link
Collaborator

can i get reasons for this option?

@lchenut
Copy link
Author

lchenut commented Sep 29, 2022

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.

@arnetheduck
Copy link
Member

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)

@cheatfate
Copy link
Collaborator

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 chronos just because libp2p tests could not properly call close procedure.

@Menduist
Copy link
Contributor

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

@lchenut
Copy link
Author

lchenut commented Sep 30, 2022

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.

@cheatfate
Copy link
Collaborator

We know that the tracker is leaking, the point is to reset the state before other tests, otherwise all the following tests will fail.

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 suite and run initialization and clean up in suite block.

@arnetheduck
Copy link
Member

clean up

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.

@cheatfate
Copy link
Collaborator

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.

@arnetheduck
Copy link
Member

improve readability when a tracker

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 unittest2 or somewhere.

@Menduist
Copy link
Contributor

Also see #326, which opens the question "should closing the dispatcher close the associated FDs" (which could have been moved elsewhere)

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.

4 participants