vuejs / vue

This is the repo for Vue 2. For Vue 3, go to https://github.com/vuejs/core
http://v2.vuejs.org
MIT License
207.46k stars 33.64k forks source link

test named "vdom patch: children" occasionally fails #10530

Open Siegrift opened 4 years ago

Siegrift commented 4 years ago

Version

2.6.10

Reproduction link

https://circleci.com/gh/vuejs/vue/13744?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Steps to reproduce

Tests are flaky on CI and also sometimes locally. Re-running the tests seem to work

What is expected?

Tests not to be flaky

What is actually happening?

Tests are flaky

posva commented 4 years ago

The test have been passing before without problems, so it may be something within the PR causing the test to fail, and in that case, tests should be updated within the PR.

Maybe someone will pick this up but the Tests not to be flaky description is far from giving direction on when would you expect the test to fail or not

Siegrift commented 4 years ago

Well, I don't think they are related. Also If I try to run yarn test couple of times, I can see some flaky tests.

But it's true that the description is rather vague. But I don't really know what is wrong, but someone else might :)

mbpreble commented 4 years ago

I've seen this test fail locally (by using fdescribe to just run this suite) but I don't have a great handle on frequency since it was running in a loop without a counter.

Have not yet seen it fail when running in isolation(fit on the shuffle test) after 50 runs in a loop.

Tough to tell what's going wrong in a test that is explicitly randomized and never quite testing the same thing.

I've at least set up a harness that will allow me to run the suite until failure and capture some debugging logs, hopefully that will help clear things up.

mbpreble commented 4 years ago

The issue seems to be when the test generates an opacity value in the inclusive range 0.00001-0.00009. The assertion does not work because the patched element ends up with an opacity which is in scientific notation (e.g. 1e-05). With enough pulls on the slot machine this test will eventually produce such a value and fail.

Here's a minimal test which reproduces the "problem". It fails with Expected '1e-05' to be '0.00001'.

import VNode from 'core/vdom/vnode'
import { patch } from 'web/runtime/patch'
it('should reproduce the flaky test', () => {
  const vnode1 = new VNode('div', { style: { opacity: '1' }})
  const vnode2 = new VNode('div', { style: { opacity: '0.00001' }})
  patch(null, vnode1)
  const elm = patch(vnode1, vnode2)
  expect(elm.style.opacity).toBe('0.00001')
})

If this is in fact the desired behavior and this test is useful, it should be possible to fix it by using less precision (four digits after the decimal) for the random opacity values.

mbpreble commented 4 years ago

Created a minimal PR to address. It reduce the number of post-decimal digits allowed in the random opacity values to 4, cutting out values in the range that case the test to fail. The previously failing assertion has been replaced with an equivalent, clearer assertion.

mbpreble commented 4 years ago

@posva Any interest in the patch here? Doing some math here, the random chance of failure for any given run of the test suite due to this issue is about 0.6% (70 chances to pull one of 9 'bad' values out of a pool of 100K). It doesn't seem likely to happen all that often, but if it's patched it shouldn't fail at all.