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.

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

CLDR-17287 basic performance test for the back end #3418

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Dec 13, 2023

  • execute a bunch of votes all in a row, which calls VoteResolverAPI (refactored) from the unit test to vote and to check the voting results.

CLDR-17287

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

- execute a bunch of votes all in a row, which calls STFactory.PerLocaleData.voteForvalueWithType()
@srl295 srl295 requested a review from btangmu December 13, 2023 20:55
@srl295 srl295 self-assigned this Dec 13, 2023
@srl295
Copy link
Member Author

srl295 commented Dec 13, 2023

@btangmu this is a start, i'm redoing it to go through VoteAPIHelper!

- refactor VoteAPIHelper to be callable from unit tests outside of a web server
- improve error results from ST
@srl295 srl295 marked this pull request as ready for review December 14, 2023 18:25
@srl295
Copy link
Member Author

srl295 commented Dec 14, 2023

@btangmu i'm getting the 'read' side going too!

btangmu
btangmu previously approved these changes Dec 14, 2023
Copy link
Member

@btangmu btangmu left a comment

Choose a reason for hiding this comment

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

ok if TODOs get done in subsequent pr

- update apiverify to verify via API
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/api/STError.java is different
  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/api/VoteAPIHelper.java is different
  • tools/cldr-apps/src/main/java/org/unicode/cldr/web/CookieSession.java is now changed in the branch
  • tools/cldr-apps/src/test/java/org/unicode/cldr/unittest/web/TestSTFactory.java is different
  • tools/cldr-apps/src/test/resources/org/unicode/cldr/unittest/web/data/TestVotePerf.xml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295 srl295 requested a review from btangmu December 14, 2023 21:01
Copy link
Member

@btangmu btangmu left a comment

Choose a reason for hiding this comment

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

approving reluctantly, see comment about deep nesting due to unnamed handler that needs to be named instead of being stuffed inside the parentheses in myReader.setHandler(...)

break;
default:
throw new IllegalArgumentException(
"Unknown test element type " + elem);
}
}

private void handleElementVerify(
Copy link
Member

Choose a reason for hiding this comment

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

this code is too deeply nested! the root of the problem starts with myReader.setHandler(...) followed by all the code for the unnamed handler being within the parentheses. it would be sooooo much better to give the handler a name, and just put its name in the parentheses, and define the handler as a named class outside of runDataDrivenTest to prevent runDataDrivenTest from being 600+ lines long

the deep nesting probably causes the formatter to insert more newlines, adding insult to injury

@srl295 srl295 merged commit 50e6e41 into unicode-org:main Dec 14, 2023
10 checks passed
@srl295 srl295 deleted the cldr-17287/be-perftest branch December 14, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants