The Wayback Machine - https://web.archive.org/web/20201203080901/https://github.com/jsperf/jsperf.com/pull/464
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

Fix issue with bad updates causing disappearing tests #464

Merged
merged 1 commit into from Mar 11, 2018

Conversation

@swang
Copy link
Contributor

@swang swang commented Mar 10, 2018

Fixes #236

Pretty much similar to #414, but with an added test to show the issue.

Based on what @mathiasbynens said in that PR #414 (comment)

It seems like the original fix in #414 was correct. I have also written a test that shows the correct action.

If you are the owner of the test (aka you own the 1st revision), only you (with the isOwn flag set to true) should be able to "update" an existing test. Otherwise you are not the owner and you should only be inserting your test cases into a revision.

The reason for all the tests having "empty" test cases is because the code would fall into the UPDATE sql statement every single time regardless of whether you're the actual owner or not because each test always passes a testID unless a new test case is added to your revision of the test.
E.g. every test has the two basic test cases, and so if you try to create a new revision and only edit those 2 test cases, you'll get a blank test revision. If you add an extra test case in that revision, then that one won't have a testID and thus will make an INSERT call and this new revision will only have the new test case.

The additional test case shows that if you are the owner, you make updates if the test cases exist, and if you are not the owner you only make inserts.

Fixes #236

Pretty much similar to #414, but with an added test to show issue.
@swang swang force-pushed the swang:fix_236_with_tests branch from 66a7b1d to f8529ae Mar 10, 2018
@maxbeatty maxbeatty requested review from mathiasbynens and asilluron Mar 10, 2018
@mathiasbynens mathiasbynens merged commit f144e66 into jsperf:master Mar 11, 2018
3 of 4 checks passed
3 of 4 checks passed
Node Security 40 vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
security/snyk No dependency changes
Details
@mathiasbynens
Copy link
Contributor

@mathiasbynens mathiasbynens commented Mar 11, 2018

Thank you!

@CTimmerman
Copy link

@CTimmerman CTimmerman commented Jul 19, 2018

Still not working on https://jsperf.com/arr-map-filter-reduce/4/edit - Only my added test is inserted and the others are lost, even when updated.

@bthallion
Copy link

@bthallion bthallion commented Aug 5, 2018

I'm having the same issue, editing a test suite and not having the exisiting tests added, for this suite https://jsperf.com/preallocating-array

maxbeatty added a commit that referenced this pull request Oct 9, 2018
* master:
  Adding X-UA-Compatible in the head (#471)
  fix: .snyk & package.json to reduce vulnerabilities (#477)
  add link to wiki (#466)
  Fix issue with bad updates causing disappearing tests (#464)
  Fix cache start (#453)
  Ensure that edited test cases can be set as synchronous/asynchronous 
(#451)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.