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

Add unit test suite for CaseMap #3213

Merged
merged 1 commit into from
Feb 6, 2016
Merged

Add unit test suite for CaseMap #3213

merged 1 commit into from
Feb 6, 2016

Conversation

TimvdLippe
Copy link
Contributor

Added a testsuite to test the case-map functionality. Includes a testcase for #3206. Therefore on current master the CI fails, but after a fix has been submitted (in #3279) the CI will succeed.

@TimvdLippe TimvdLippe force-pushed the fix-consecutive-uppercase-casemap branch from acb0c82 to 8cc447d Compare December 18, 2015 00:54
@TimvdLippe TimvdLippe mentioned this pull request Dec 18, 2015
Closed
@TimvdLippe TimvdLippe mentioned this pull request Jan 9, 2016
Merged
@TimvdLippe
Copy link
Contributor Author

Waiting for the response on #3279. If that PR gets merged, I will close this one.

@sorvell sorvell added the p0 label Feb 4, 2016
@kevinpschaaf
Copy link
Member

#3208 resolves this issue also; @azakus will review both and choose the 🏆 winner 🏆!

@TimvdLippe
Copy link
Contributor Author

@azakus I did the performance test as created by @nazar-pc in #3279 to compare the result. My base-case was consistenly twice as fast as nazar's, so that would be hugely dependent of the hardware.

before-1: 500.837ms
before-2: 508.146ms
after-1: 244.371ms
after-2: 382.750ms
after-nazar-1: 240.780ms
after-nazar-2: 221.723ms

Executing the test multiple times, nazar's implementation proved to be slightly faster. Therefore I will change this PR to just include the tests and then the implementation of nazar could be merged (as this also fixes #3206.

@TimvdLippe TimvdLippe force-pushed the fix-consecutive-uppercase-casemap branch from 8cc447d to 3d7eb8b Compare February 4, 2016 09:51
@TimvdLippe
Copy link
Contributor Author

The CI build currently fails, as the original is not fixed. When #3279 is merged and this branch is rebased, the build will succeed.

@TimvdLippe TimvdLippe changed the title Fix consecutive uppercase characters in camelCase conversion Add unit test suite for CaseMap Feb 4, 2016
@TimvdLippe TimvdLippe force-pushed the fix-consecutive-uppercase-casemap branch from 3d7eb8b to ee9a600 Compare February 5, 2016 13:15
@dfreedm
Copy link
Member

dfreedm commented Feb 6, 2016

@TimvdLippe Thanks for the perf test! LGTM

@dfreedm
Copy link
Member

dfreedm commented Feb 6, 2016

#3279 merged, merging this now.

dfreedm added a commit that referenced this pull request Feb 6, 2016
@dfreedm
Merge pull request #3213 from TimvdLippe/fix-consecutive-uppercase-ca…
040787c 
…semap

Add unit test suite for CaseMap
@dfreedm dfreedm merged commit 040787c into Polymer:master Feb 6, 2016
@dfreedm
Copy link
Member

dfreedm commented Feb 6, 2016

Ha, I was a bit hasty. This won't get run automatically, but must be listed in test/runner.html. I'll do that in a different commit.

@TimvdLippe
Copy link
Contributor Author

Oops yes you are correct. Sorry!

dfreedm added a commit that referenced this pull request Feb 6, 2016
@dfreedm
Actually execute case-map
d84d75b 
This was missing from #3213
@TimvdLippe TimvdLippe deleted the fix-consecutive-uppercase-casemap branch October 5, 2017 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dfreedm dfreedm

Labels
cla: yes p0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TimvdLippe @kevinpschaaf @dfreedm @sorvell @googlebot