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(challenge): remove type coercion check for true and false return #36519

Merged
merged 2 commits into from
Jul 30, 2019

Conversation

lasjorg
Copy link
Contributor

@lasjorg lasjorg commented Jul 26, 2019

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

This PR removes the type coercion from the test and adds strict checking for false/true return values.

The current test will coerce an undefined return value from the function, making both "false" tests pass right out of the gate. I don't think this is a good starting point. All tests should fail by default and not give the user the wrong impression that their function is passing some of the tests even if they are not returning anything. Returning truthy/falsy values (other then true/false) should not make any test pass/fail.

@lasjorg
Copy link
Contributor Author

lasjorg commented Jul 26, 2019

I guess maybe I should have used the Chai methods (isTrue/isFalse). Is it preferred to use chai methods when possible?

There doesn't seem to be any mention of it in the style guide. If it is preferred maybe we should add that to the Writing tests part of the style guide?

@camperbot camperbot added language: English scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. labels Jul 29, 2019
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ojeytonwilliams
Copy link
Contributor

@lasjorg regarding Chai, I think it's always used. The assert object is just Chai's assert, rather than Node's.

A PR fixing the docs would be great if you have the time. Let me know, either way.

@lasjorg
Copy link
Contributor Author

lasjorg commented Jul 30, 2019

@ojeytonwilliams I just meant if Chai has an assert method, like isTrue, deepEqual, etc. if its usage should be encouraged over making your own test expressions. And If so, if this should be mentioned in the Writing tests part of the style guide.

For it to have any meaningful effect I guess it would need to be, at least loosely, enforced as part of the code review process as well. I'm just thinking if enforcing/encouraging it might potentially help make tests a bit cleaner.

Not sure if I should make an issue and ask for feedback on this?

@ojeytonwilliams
Copy link
Contributor

Oh, I see what you're saying. I don't think we want to be too dogmatic, but encouraging their use can't hurt.

That said, making an issue's a reasonable idea, just for visibility. So, please go ahead.

Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

LGTM @lasjorg 🎉 Thanks for contributing 🎉

@moT01 moT01 merged commit 881be71 into freeCodeCamp:master Jul 30, 2019
@lasjorg lasjorg deleted the fix/use-the-every-method-to-check branch November 5, 2019 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants