Skip to content

Fix repeatedly failing windows test timeouts #5340

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.

Sign up for GitHub

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

Merged
mattseddon merged 3 commits into from
Feb 27, 2024
Merged

Fix repeatedly failing windows test timeouts #5340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18231f0
Increase timeout on windows tests
julieg18 Feb 26, 2024
90c36c2
fix test describe nesting and timeouts
julieg18 Feb 27, 2024
4fc6f37
Merge branch 'main' into fix-windows-tests-timeouts
mattseddon Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion extension/src/test/suite/cli/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ suite('CLI Suite', () => {
})
)
expect(killed).to.be.true
})
}).timeout(10000)

it('should cleanup all non-background processes on dispose', async () => {
const processStarted = disposable.track(new EventEmitter<CliEvent>())
Expand Down
4 changes: 2 additions & 2 deletions extension/src/test/suite/experiments/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2036,8 +2036,8 @@ suite('Experiments Test Suite', () => {
['main', 'other-branch']
])
expect(sorts).to.deep.equal([{ descending: true, path: paramPath }])
})
}).timeout(WEBVIEW_TEST_TIMEOUT)
}).timeout(WEBVIEW_TEST_TIMEOUT)
})

describe('persisted state', () => {
const firstSortDefinition = {
Expand Down
2 changes: 1 addition & 1 deletion extension/src/test/suite/experiments/model/tree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ suite('Experiments Tree Test Suite', () => {

expect(mockExpPush).to.be.calledWithExactly(dvcDemoPath, mockExperimentId)
expect(mockUpdate).to.be.calledOnce
})
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should be able to push the provided experiment with dvc.views.experimentsTree.pushExperiment (if no experiments are selected)', async () => {
bypassProgressCloseDelay()
Expand Down
4 changes: 2 additions & 2 deletions extension/src/test/suite/experiments/workspace.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ suite('Workspace Experiments Test Suite', () => {

await testFile('params.yaml')
await testFile('dvc.yaml')
})
}).timeout(WEBVIEW_TEST_TIMEOUT)
}).timeout(WEBVIEW_TEST_TIMEOUT)
Copy link
Contributor Author

@julieg18 julieg18 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be an issue with tests inside of describe blocks not getting the timeout. This test was failing with a timeout of 6000 until I moved the .timeout. This PR is also failing with:

  1) Experiments Test Suite
       Sorting
         should not show duplicate rows when sorted:
     Error: Timeout of 6000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (d:\a\vscode-dvc\vscode-dvc\extension\dist\test\suite\experiments\index.test.js)
  	at listOnTimeout (node:internal/timers:569:17)
  	at process.processTimers (node:internal/timers:512:7)

This sorting test is also relying on its parent describe block for its set webview timeout.

Copy link
Contributor Author

@julieg18 julieg18 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed mocha prs and issues, but not seeing anything. It's also strange that we're hitting all the timeouts today when we haven't merged anything recently related to mocha (as far as I can tell).

Copy link
Contributor

@mattseddon mattseddon Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess there shouldn't be multiple describe blocks nested (see below)

Copy link
Contributor Author

@julieg18 julieg18 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't there be multiple describe blocks in a block? The mocha docs show examples of this. Or am I misunderstanding something?

Copy link
Contributor

@mattseddon mattseddon Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't there be multiple describe blocks in a block? The mocha docs show examples of this. Or am I misunderstanding something?

Have a look at this and this. For our purposes, we want to avoid extra complexity/parallelization. I have very little faith that suite -> describe -> describe -> it acts the same way as suite -> describe -> it

This sorting test is also relying on its parent describe block for its set webview timeout.

describe blocks should not have .timeout added to them. If you see it that is an error.

Copy link
Contributor Author

@julieg18 julieg18 Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a look at this and this. For our purposes, we want to avoid extra complexity/parallelization. I have very little faith that suite -> describe -> describe -> it acts the same way as suite -> describe -> it

Makes sense, thanks for the clarifications! I'm only seeing one example of suite -> describe -> describe -> which is our Custom Plots Creation. None of those tests have failed that I can see, but it would still be quick to remove the extra nesting there.

describe blocks should not have .timeout added to them. If you see it that is an error.

Oh, ok that explains the issue. I actually couldn't find documentation on .timeout which explains how I missed this 🤦‍♀️.

The tests appear to be running normally again but I'll go ahead and fix the issues mentioned above to ensure this doesn't happen again.

})

describe('dvc.modifyWorkspaceParamsAndQueue', () => {
it('should be able to queue an experiment using an existing one as a base', async () => {
Expand Down
110 changes: 54 additions & 56 deletions extension/src/test/suite/plots/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,81 +180,79 @@ suite('Plots Test Suite', () => {
)
})

describe('Custom Plots Creation', () => {
it('should only use unfiltered experiments and commits in custom plots', async () => {
const { plots, plotsModel, experimentsModel } = await buildPlots({
disposer: disposable,
plotsDiff: plotsDiffFixture
})
it('should only use unfiltered experiments and commits in custom plots', async () => {
const { plots, plotsModel, experimentsModel } = await buildPlots({
disposer: disposable,
plotsDiff: plotsDiffFixture
})

const plotsCustomPlotsSpy = spy(PlotsCollectUtils, 'collectCustomPlots')
const plotsCustomPlotsSpy = spy(PlotsCollectUtils, 'collectCustomPlots')

await plots.isReady()
await plots.isReady()

stub(experimentsModel, 'getFilters')
.onFirstCall()
.returns([
{
operator: Operator.EQUAL,
path: 'params:params.yaml:epochs',
value: 2
}
])
stub(experimentsModel, 'getFilters')
.onFirstCall()
.returns([
{
operator: Operator.EQUAL,
path: 'params:params.yaml:epochs',
value: 2
}
])

plotsModel.getCustomPlots()
plotsModel.getCustomPlots()

const allExperiments: Experiment[] =
experimentsModel.getWorkspaceCommitsAndExperiments()
const allExperiments: Experiment[] =
experimentsModel.getWorkspaceCommitsAndExperiments()

const { experiments } = plotsCustomPlotsSpy.firstCall.args[0]
const { experiments } = plotsCustomPlotsSpy.firstCall.args[0]

expect(experiments).to.deep.equal(
allExperiments.filter(
({ id, params }) =>
id !== EXPERIMENT_WORKSPACE_ID &&
params?.['params.yaml']?.epochs === 2
)
expect(experiments).to.deep.equal(
allExperiments.filter(
({ id, params }) =>
id !== EXPERIMENT_WORKSPACE_ID &&
params?.['params.yaml']?.epochs === 2
)
})
)
})

it('should handle all experiments/commits being filtered', async () => {
const { plots, plotsModel, experimentsModel } = await buildPlots({
disposer: disposable,
plotsDiff: plotsDiffFixture
})
it('should handle sending custom plots when all experiments/commits being filtered', async () => {
const { plots, plotsModel, experimentsModel } = await buildPlots({
disposer: disposable,
plotsDiff: plotsDiffFixture
})

await plots.isReady()
await plots.isReady()

stub(experimentsModel, 'getUnfilteredCommitsAndExperiments')
.onFirstCall()
.returns([])
stub(experimentsModel, 'getUnfilteredCommitsAndExperiments')
.onFirstCall()
.returns([])

const customPlots = plotsModel.getCustomPlots()
const customPlots = plotsModel.getCustomPlots()

expect(customPlots).to.deep.equal({
...customPlotsFixture,
hasUnfilteredExperiments: false,
plots: []
})
expect(customPlots).to.deep.equal({
...customPlotsFixture,
hasUnfilteredExperiments: false,
plots: []
})
})

it('should handle no plots being added yet', async () => {
const { plots, plotsModel } = await buildPlots({
disposer: disposable,
plotsDiff: plotsDiffFixture
})
it('should handle sending custom plots when no plots have been added yet', async () => {
const { plots, plotsModel } = await buildPlots({
disposer: disposable,
plotsDiff: plotsDiffFixture
})

await plots.isReady()
await plots.isReady()

stub(plotsModel, 'getCustomPlotsOrder').onFirstCall().returns([])
stub(plotsModel, 'getCustomPlotsOrder').onFirstCall().returns([])

const customPlots = plotsModel.getCustomPlots()
const customPlots = plotsModel.getCustomPlots()

expect(customPlots).to.deep.equal({
...customPlotsFixture,
hasAddedPlots: false,
plots: []
})
expect(customPlots).to.deep.equal({
...customPlotsFixture,
hasAddedPlots: false,
plots: []
})
})

Expand Down