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

Make default TestAgency URI unique per run #931

Closed
wants to merge 3 commits into from

Conversation

ChrisMaddock
Copy link
Member

Currently, it's not possible to instantiate an engine inside a test run, due to two TestAgency's colliding on the same URI. This causes problems when testing runners and the engine, and requires injecting a second URI into the test TestAgency, which isn't possible for runners accessing the engine through the ITestEngine interface.

As far as I can think - a TestAgency URI needs to be fixed-per-run, but there's no requirement for it to be fixed across different runs. I think adding a GUID to the default TestAgency URI would be a simple change to resolve this problem, and make it easier for runners to test?

@CharliePoole - you've been working in this area recently - do you agree with my logic here?

@ChrisMaddock ChrisMaddock added this to the 3.13 milestone Apr 4, 2021
@ChrisMaddock ChrisMaddock self-assigned this Apr 4, 2021
@CharliePoole
Copy link
Collaborator

You may be on to something, but I don't understand it well enough to be sure. What if the same port is chosen? Is it OK to have the same port so long as the uri portion is different? Can we test that by temporarily using a fixed value for the port?

Copy link
Collaborator

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

I'd like to see a test, which is known to fail without this change, but which passes when it's in place. My guess is that simply trying to start a second-level agency should cause an exception without this fix. Would you mind trying that?

@ChrisMaddock
Copy link
Member Author

Yeah, that's a good point. I just did this quickly through the GitHub GUI following the adapter issue - I'll pull it down and add a test, it'll be simple enough. I think we also have some tests that include workarounds for this issue - I could remove those at the same time.

@ChrisMaddock
Copy link
Member Author

Ok, have filled this one out a little. 🙂

Added a test that failed before the change and passes afterwards - have tested this manually. Also removed the workarounds in out own test suite.

This is good for review, but I've marked it blocked as I don't want to cause any merge conflicts with #937. Let's get that merged in first, and I'll rebase this one if required. 😄

@ChrisMaddock
Copy link
Member Author

Never mind...the test seems to be failing on the netcore build only with a mysterious NullReferenceException. That's the end of my time for today - this one will have to wait. 🙂

@ChrisMaddock
Copy link
Member Author

Replaced by #942

@ChrisMaddock ChrisMaddock deleted the ChrisMaddock-patch-1 branch May 9, 2021 15:32
@CharliePoole CharliePoole modified the milestones: 3.13, Closed Without Action Nov 7, 2021
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.

2 participants