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

notDeepLoosEqual to set .notExpected property instead of .expected #191

Closed
wants to merge 1 commit into from
Closed

Conversation

hville
Copy link

@hville hville commented Sep 17, 2015

Unlike t.notEqual and t.notDeepEqual, t.notDeepLooseEqual sets the reference value in the .expected property instead of the .notExpected property.

To be consistent with notEqual and notDeepEqual
@ljharb
Copy link
Collaborator

ljharb commented Sep 17, 2015

This would be a breaking change, and it has no tests.

If this change isn't causing any tests to fail, tests will need to be added - otherwise, they'd need to be modified.

@hville
Copy link
Author

hville commented Sep 19, 2015

The PR passes all of the current tests and is consistent with all other methods.

Creating a new test for this would invlove also creating new tests for deepEqual, notDeepEqual, deepLooseEqual and notDeepLooseEqual since the .expected and .notExpected properties are not validated in any of the current tests. This would be a seperate PR with a much wider scope.

@ljharb
Copy link
Collaborator

ljharb commented Sep 19, 2015

It's not solely up to me, but I'd rather refuse to merge a PR that doesn't come with at least a regression test. Even a single test case, that breaks without your change, would be a start. If tests are passing without your change - and your change is necessary - then the lack of a failing test is part of the same bug.

@hville
Copy link
Author

hville commented Sep 19, 2015

Ok. Let me think about it because I don't really know what to test apart from the property itself. (I'm using these properties to pretty-format the output from the native result object instead of the text output)

@Raynos
Copy link
Collaborator

Raynos commented Sep 21, 2015

lgtm.

@hville
Copy link
Author

hville commented Oct 24, 2015

Checked the TAP spec and node's assert to compare and devise relevant tests and found that node's assert only uses the .expected property for all assertion types instead of .expected and .notExpected. For consistency I would be better to change tape's properties accordingly (the opposite of the fix proposed in this PR) which could be a breaking change. I'm now using assert directly instead of jugling arround with tape's stream so this PR can be dropped and closed.

@ljharb
Copy link
Collaborator

ljharb commented Jan 9, 2019

This seems subsumed by #454.

@ljharb ljharb closed this Jan 9, 2019
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.

3 participants