Unstable test "should reload config on change"

[raman-m]

Expected Behavior

The test should_reload_config_on_change doesn't fail in 100% of running

Actual Behavior / Motivation for New Feature

Currently the test should_reload_config_on_change fails sometimes: in 1/3 or even 1/2 cases. Possible reason: multithreading for file access when test class tests being run in parallel.

Steps to Reproduce the Problem

develop builds:

• Build 1666: Scenario: should reload config on change
• Build 1592: Scenario: should reload config on change

PR builds:

• Build 1710: Scenario: should reload config on change
• etc.

Specifications

• Version: latest
• Platform: Docker
• Subsystem: CircleCI

[raman-m]

@ggnaegi Hey Guillaume! Could you look into this problem, please?

[ggnaegi]

@raman-m Ok, we have a problem here. How could I possibly reproduce the same conditions as the CI Pipeline?

In xUnit, tests (facts) are designed to be run in isolation, which means there's no guaranteed order of execution. This is intentional and follows the unit testing best practice of ensuring that each test is independent and doesn't rely on state changes from other tests.

[ggnaegi]

@raman-m What we could do, is provide a factory for the configuration files and change the configuration files id depending on the facts

[raman-m]

@ggnaegi I see 2-3 possible solutions:

1. Make test class tests running sequentially via good known xUnit attribute. Most easy and fast fix delivery. But it seems you don't like this solution.

2. If you want to make each test totally independent then we have to abstract from hardcoded ocelot.json file, and apply test id prefix/suffix in the file name to make tests independent.

3. On the level of all acceptance tests create multithreading synchronization primitive for access to ocelot.json file, and apply multithreading lock of the file for these tests.

1st solution is sequential. 2nd and 3rd are multithreaded/ parallel.

Complexity: 1st is the simplest. 2nd is a compromise, it is not complex but requires writing some code. 3rd is the most complex, it requires to change at least all tests which reads/writes from/to ocelot.json file.

I'm okay with all three solutions because they fix stability of the test. But they require different efforts to implement, like time & coding.

Please, choose the favorite solution which you're liking...

[ggnaegi]

@raman-m 1 is ok, but xunit assumes the facts in the same class are isolated and side effects free. So it would mean, we would have to refactor the code like that:

[Collection("Sequential")]
public class Sequential1
{
    [Fact]
    public void should_reload_config_on_change() { /* ... */ }
}

[Collection("Sequential")]
public class Sequential2
{
    [Fact]
    public void should_not_reload_config_on_change() { /* ... */ }
}

It's a bit ugly, no. So that is why I came up with the factory idea...

[raman-m]

@ggnaegi What are the side effects?

You can simply extract all tests that use ocelot.json to a separate class and disable parallelizm on them. It seems you have to...

[ggnaegi]

@raman-m Yeah well, it's sunday, it's mistakes prone... Maybe we should only make sure the facts are using their own ocelot.json file? And then cleanup?

[raman-m]

I like the 2nd solution to run test on a separate test file. Did you count tests which use ocelot.json file?

[ggnaegi]

I like the 2nd solution to run test on a separate test file. Did you count tests which use ocelot.json file?

@raman-m We have 168 references toGivenThereIsAConfiguration.... So I'm quite surprised that we haven't other tests failing...

[ggnaegi]

In xUnit, each [Fact] or [Theory] method gets a fresh instance of the test class. This means that any instance variables you've declared in the class are re-instantiated for each test method.

So, the Steps class is re-instantiated for each test method. So each fact/theory has his own Steps instance. Maybe we could use a uuid in Steps class and create a path like uuid-ocelot.json

In Dispose, we would delete the uuid-ocelot.json file.

@raman-m I have the feeling, that would be a good solution...

[raman-m]

@ggnaegi branch bug-1700 The problem is not in parallel execution! I've applied a quick fix to disable parallelization, but it didn't help. Probably all tests which use ocelot.json must be converted to sequential... There will be no hotfix :(

[ggnaegi]

@raman-m As you can see, I can reproduce the error with Open_circuit_should_not_effect_different_route(). After 133 iterations it eventually failed. The good news is, in the last 20 tests I started, the error with reload didn't occur...

[raman-m]

@ggnaegi commented on Sep 25:

As you can see, I can reproduce the error with Open_circuit_should_not_effect_different_route(). After 133 iterations it eventually failed.

How did you run so many iterations? Let's create new issue and make new PR after delivery of this bug fix. But, it is not urgent!

[ggnaegi]

@raman-m commented on Sep 25

-> Test Explorer -> Run Until Failure

[raman-m]

@ggnaegi commented on Sep 25:

-> Test Explorer -> Run Until Failure

OK! 😃 And FYI, I've created #1706