Skip to content

Conversation

@mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Jul 12, 2022

2/2 main <- #2002 <- this

This PR adds an end to end test for the SCM view and our custom file decorations.

@mattseddon mattseddon self-assigned this Jul 12, 2022
@mattseddon mattseddon changed the base branch from main to run-e2e July 12, 2022 05:13
@mattseddon mattseddon marked this pull request as ready for review July 12, 2022 06:41
expect(expectedScmItemLabels.sort()).toStrictEqual(
dvcTreeItemLabels.sort()
)
}).timeout(60000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this timeout too long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SCM state relies on dvc status. Whenever an experiment is running dvc status will fail. There is an exponential back off in place for retries which means that after the experiment finishes there can be a significant lag before the SCM view is updated with the correct details. Locally this is less of a problem but in the CI running an experiment takes longer. Leading to a longer lag in the subsequent update. I'd rather that this test is reliable with a longer timeout than flaky with a shorter one.

Does that make sense? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does for me 👍
I know this is a can of worms, but is there a way to only dvc status after all the runs? Like using a subscription or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: can it be done as a global setting - one minute timeout per test?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably it applies to all tests that run DVC internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is a can of worms, but is there a way to only dvc status after all the runs? Like using a subscription or something like that?

That would mean we would have to introduce stubs into this test suite, making them less e2e. I would rather test the extension as is. We should be able to improve the situation once we get data:status.

Q: can it be done as a global setting - one minute timeout per test?

This was actually already set to 1m

Base automatically changed from run-e2e to main July 12, 2022 23:50
@mattseddon mattseddon enabled auto-merge (squash) July 13, 2022 00:11
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit dbbba1e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit a6f43eb into main Jul 13, 2022
@mattseddon mattseddon deleted the add-e2e-test branch July 13, 2022 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants