autotest / autotest-docker

Autotest client test containing functional & integration subtests for the Docker project
Other
25 stars 30 forks source link

wait test is non-deterministic #120

Closed cevich closed 10 years ago

cevich commented 10 years ago

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 commented 10 years ago

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

ldoktor commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@ldoktor can we get a PR for this please?

cevich commented 10 years ago

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 commented 10 years ago

This is being addressed in https://github.com/autotest/autotest-docker/pull/158