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

wait test is non-deterministic #120

Closed
cevich opened this issue May 1, 2014 · 9 comments · Fixed by #158
Closed

wait test is non-deterministic #120

cevich opened this issue May 1, 2014 · 9 comments · Fixed by #158

Comments

@cevich
Copy link
Member

cevich commented May 1, 2014

This method changes testing behaviour randomly:

def init_use_names(self, use_names=False):
    if use_names:
        conts = self.sub_stuff['containers']
        containers = DockerContainers(self.parent_subtest)
        containers = containers.get_container_list()
        cont_ids = [cont['id'] for cont in conts]
        for cont in containers:
            if cont.long_id in cont_ids:
                if use_names is not True and random.choice((True, False)):
                    continue    # 50% chance of using id vs. name
                # replace the id with name
                cont_idx = cont_ids.index(cont.long_id)
                conts[cont_idx]['id'] = cont.container_name

If there's a bug in either one, and one-shot to find it on a test-run, this test will fail 50% of the time. It also makes debugging really difficult since the precise behaviour is always somewhat undefined.

@cevich
Copy link
Member Author

cevich commented May 1, 2014

@ldoktor since you wrote this test, could you fix this please?

@ldoktor
Copy link
Member

ldoktor commented May 2, 2014

Yes, it's non-deterministic by default. Anyway you can specify whether to use names in the config (use_names = true/false/RANDOM, it's described in documentation and config).

@ldoktor
Copy link
Member

ldoktor commented May 2, 2014

btw I never seen this test fail. Could you please provide the log? (I'm using docker-io-0.10.0-2.fc20.x86_64)

@cevich
Copy link
Member Author

cevich commented May 2, 2014

Sometimes you only get one-chance to find a bug, so our tests need to be have predictable behaviour. If using names or id's is an important distinction (for the present), we need to test both. If it's not important right now, then there's no need to have an option. My suggestion is do both, or pick one and hard-code it. Later if it becomes important, it's easy to add, and in the meantime the test is less complex. In either case, the test will remain at least 2x more predictable beyond all the random-crap we can't predict/control. That's why we need this fixed.

@ldoktor
Copy link
Member

ldoktor commented May 5, 2014

That's actually why I really miss cartesian config. It's amazing for these kind of jobs, one test, 2 variants and that's it. (--sig-proxy config would be just 2 config variants (resulting in 8 tests, or 3 variants with 12 tests covering all alternatives)...

I put the randomness here because I take this test as basic usage test, which runs with each build and could uncover basic odd behaviors. When we discover, that naming has problems, we can create specific tests, which would specialize only on the simple reproducer so we won't need to run this time-consuming test twice just because name-translation problem.

Anyway I can implement everything, so just tell me which one you prefer...

@cevich
Copy link
Member Author

cevich commented May 5, 2014

Hmmm, eventually we may want to test it both ways, but probably just hard-coding it for "name" now is fine. At this stage, I think we're still all about wide-coverage as opposed to trying to exercise every little nook-and-cranny possible. So just hitting the major points is fine for now.

@cevich
Copy link
Member Author

cevich commented May 23, 2014

@ldoktor can we get a PR for this please?

@cevich
Copy link
Member Author

cevich commented May 30, 2014

Bumping this to 0.7.5 cause I want to get 0.7.4 done and there's no code for this issue in a PR yet.

@cevich
Copy link
Member Author

cevich commented Jun 5, 2014

This is being addressed in #158

@cevich cevich closed this as completed Jun 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants