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

Use IAsyncLifetime in TorTests & LiveServerTests. #4324

Merged
merged 2 commits into from Sep 15, 2020

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Sep 8, 2020

This is part of #4244 project where I would actually like to get rid of those await Task.Delay(3000); delays in TorTests and enforce that when you start Tor and you ask for ensureRunning it will be started or an exception will be thrown. This PR paves the path to that goal.

This PR attempts to rewrite TorTests & LiveServerTests initialization using proper mechanism provided by xUnit - namely IAsyncLifetime interface: https://mderriey.com/2017/09/04/async-lifetime-with-xunit/

PR is in conflict with #4305. That PR should be merged first and I'll modify this one after that.

Notes:

  • This PR changes integration tests so azure pipelines won't catch any error. To test it, you need to run the affected integration tests.

var torManager = new TorProcessManager(Global.Instance.TorSocks5Endpoint, Global.Instance.TorLogsFile);
torManager.Start(ensureRunning: true, dataDir: Path.GetFullPath(AppContext.BaseDirectory));
Task.Delay(3000).GetAwaiter().GetResult();
await Task.Delay(3000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we ask TorProcessManager is Tor is running or not?

Copy link
Collaborator Author

@kiminuo kiminuo Sep 8, 2020

Choose a reason for hiding this comment

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

Not sure if it is helpful at this point because my PRs attempt to reach the state where we would just call: await torManager.Start(ensureRunning: true) and that would wait for Tor to be running or throw an exception that Tor is not reachable.

EDIT: And also that would remove await Task.Delay(3000);.

@yahiheb
Copy link
Collaborator

yahiheb commented Sep 10, 2020

CanRequestClearnetAsync doesn't pass for me, and it doesn't pass on master also.

@kiminuo
Copy link
Collaborator Author

kiminuo commented Sep 11, 2020

CanRequestClearnetAsync doesn't pass for me, and it doesn't pass on master also.

The test is passing for me. Does the website load in your browser?

@yahiheb
Copy link
Collaborator

yahiheb commented Sep 12, 2020

The test is passing for me. Does the website load in your browser?

Yes.

…torTests

# Conflicts:
#	WalletWasabi.Tests/IntegrationTests/LiveServerTests.cs
#	WalletWasabi.Tests/IntegrationTests/TorTests.cs
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 12, 2020
@yahiheb
Copy link
Collaborator

yahiheb commented Sep 12, 2020

LGTM

@nopara73
Copy link
Contributor

This is cool. Is this something new? I had no idea about this. With this we can probably even remove the RegTestFixture.

Copy link
Contributor

@nopara73 nopara73 left a comment

Choose a reason for hiding this comment

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

LGTM

@kiminuo
Copy link
Collaborator Author

kiminuo commented Sep 12, 2020

This is cool. Is this something new? I had no idea about this. With this we can probably even remove the RegTestFixture.

https://github.com/xunit/xunit/blob/master/src/xunit.core/IAsyncLifetime.cs seems to exist for years.

@molnard molnard merged commit 157b33d into zkSNACKs:master Sep 15, 2020
@kiminuo kiminuo deleted the feature/2020-09-08-torTests branch September 15, 2020 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants