You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
changed the title [-]vdom patch: children should handle random shuffle flaky test[/-][+]test named "vdom patch: children" occasionally fails[/+]on Sep 17, 2019
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
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.
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.
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.
@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.
Activity
[-]vdom patch: children should handle random shuffle flaky test[/-][+]test named "vdom patch: children" occasionally fails[/+]posva commentedon Sep 17, 2019
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 commentedon Sep 17, 2019
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 commentedon Oct 4, 2019
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 commentedon Oct 4, 2019
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'.
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.
test(children.spec.js): fix flaky test
mbpreble commentedon Oct 5, 2019
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 commentedon Oct 15, 2019
@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.