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

Remove duplicate tests #3222

Merged
merged 1 commit into from
Oct 1, 2021
Merged

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Sep 29, 2021

While working on adding the Temporal tests from tc39/proposal-temporal (#3002) I noticed that these tests tested the same thing as each of the branding.js tests in the same directory. This removes the duplicate tests.

@jugglinmike
Copy link
Contributor

These tests definitely concern the same behavior; thanks for pointing out the redundancy! They aren't necessarily equivalent, though.

The tests named branding.js verify more receiver values, so at first glance, they seem like the ones we ought to keep. However, those tests also omit required parameters, and for some of the methods under test (e.g. dateFromFields), that omission would trigger an indistinguishable (to Test262) TypeError. The net effect is that the tests cannot identify common implementation bugs such as incorrect step sequences or omitted steps.

For other methods (e.g. dateAdd), the omission would trigger a distinct error, so while it's a little misleading for readers, it's not as much of a concern.

We could address this by updating the branding.js tests to provide valid arguments. It's tempting to defer a change like that to a follow-up patch, but doing so would reduce coverage in the interim, so I'd rather address this issue atomically.

While working on adding the Temporal tests from tc39/proposal-temporal I
noticed that these tests tested the same thing as each of the branding.js
tests in the same directory. This removes the duplicate tests.
@ptomato
Copy link
Contributor Author

ptomato commented Sep 30, 2021

Thanks, that makes sense. I've updated the branding.js tests that correspond to the duplicate tests that I removed in this PR, for now, so that we don't lose any coverage. Maybe there should be a new issue for updating the rest of the branding tests to pass valid arguments?

@jugglinmike
Copy link
Contributor

Thanks, that makes sense. I've updated the branding.js tests that correspond to the duplicate tests that I removed in this PR, for now, so that we don't lose any coverage.

Thanks!

Maybe there should be a new issue for updating the rest of the branding tests to pass valid arguments?

Sounds about right to me. See gh-3224

@jugglinmike jugglinmike merged commit c7ca06a into tc39:main Oct 1, 2021
@ptomato ptomato deleted the duplicate-branding-tests branch October 1, 2021 00:24
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