Skip to content
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

Jump to bottom

Remove 'tryWhileSummarizing' workaround for Property-Query tests #7350

Closed
wants to merge 6 commits into from
Closed

Remove 'tryWhileSummarizing' workaround for Property-Query tests #7350

wants to merge 6 commits into from

Conversation

DLehenbauer
Copy link
Contributor

@DLehenbauer DLehenbauer commented Sep 2, 2021 •
edited
Loading

Fixes GH #7340

@github-actions github-actions bot added the area: runtime Runtime related issues label Sep 2, 2021
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Sep 2, 2021 •
edited
Loading

■ @fluidframework/base-host: No change
Metric NameBaseline SizeCompare SizeSize Diff
main.js 187.51 KB 187.51 KB ■ No change
Total Size 215.09 KB 215.09 KB ■ No change
⯆ @fluid-example/bundle-size-tests: -120 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
container.js 163.31 KB 163.2 KB ⯆ -120 Bytes
map.js 44.65 KB 44.65 KB ■ No change
matrix.js 143.45 KB 143.45 KB ■ No change
odspDriver.js 175.43 KB 175.43 KB ■ No change
odspPrefetchSnapshot.js 38.98 KB 38.98 KB ■ No change
sharedString.js 165.02 KB 165.02 KB ■ No change
Total Size 763.53 KB 763.41 KB ⯆ -120 Bytes

Baseline commit: 2099885

Generated by 🚫 dangerJS against 7b78003

@vladsud
Copy link
Contributor

vladsud commented Sep 2, 2021

Interesting, it did pass for you. Might be just luck with timing?

@DLehenbauer
Copy link
Contributor Author

@vladsud - If I'm reading the test history correctly, you hit the failure just once?

I'll give it a few more tries to see if there's evidence it's running close to it's time limit.

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vladsud
Copy link
Contributor

vladsud commented Sep 2, 2021

No, I hit it continuously when rerunning pipeline on same bits. And the moment I've submitted that last commit, it cleared. What's possible is that there were more changes in between these runs and something changed somewhere else that changed timing?
If we can't repro, I'd submit this change and wait till we get further signal.

@vladsud
Copy link
Contributor

vladsud commented Sep 3, 2021

Interesting, it hit timeout in latest retry. So the is there, maybe we are right on the boundary of that 4s timeout.
I was not able to find where we set it or how to overwrite it though

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@github-actions github-actions bot requested a review from vladsud October 1, 2021 20:08
@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer
Copy link
Contributor Author

/azp run Build - client packages

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DLehenbauer DLehenbauer marked this pull request as ready for review October 4, 2021 20:24
@DLehenbauer DLehenbauer changed the title Repo GH Issue #7340 Extend timeout of 'property-query' tests Oct 4, 2021
@DLehenbauer DLehenbauer changed the title Extend timeout of 'property-query' tests Remove 'tryWhileSummarizing' workaround for Property-Query tests Oct 4, 2021
@vladsud vladsud mentioned this pull request Oct 8, 2021
Merged
vladsud added a commit that referenced this pull request Oct 8, 2021
@vladsud
Remove OpProcessingController, use LoaderContainerTracker in property…
5fc2cbf 
…DDS tests (#7784)

Slight changes in reconnect logic (see PRs #7753, #7393) result in failures in PropertyDDS UTs.

Looking a bit deeper, I can't easily follow intentions in OpProcessingController.
For example, OpProcessingController.process(dm1) will leave all but dm1 paused. UTs do rely on that (it's pretty clearly from some of them), in other places it feels like it's unintended result. And I do not think it correctly waits for all pending activity to be flushed (as observed in above mentioned PRs).

Given that it's deprecated and repo uses LoaderContainerTracker (explicitly or implicitly through), it's time to do the move.

Some random tests fail with this change, but are addressed by increasing timeout.
We saw this problem before with these tests - see pending #7350
@DLehenbauer DLehenbauer mentioned this pull request Oct 26, 2021
Closed
@ghost ghost added the status: stale label Apr 3, 2022
@ghost
Copy link

ghost commented Apr 3, 2022

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@ghost ghost closed this Apr 11, 2022
@taylorsw04 taylorsw04 deleted the repo-7340 branch June 21, 2022 15:54
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladsud vladsud Awaiting requested review from vladsud

Assignees
No one assigned
Labels
area: dds: propertydds area: runtime Runtime related issues status: stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DLehenbauer @msfluid-bot @vladsud