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

document assert usage standards for contributors #333

Closed
jeremylt opened this issue Oct 20, 2023 · 3 comments · Fixed by #334
Closed

document assert usage standards for contributors #333

jeremylt opened this issue Oct 20, 2023 · 3 comments · Fixed by #334
Assignees
Labels
type: bug Something isn't working

Comments

@jeremylt
Copy link
Member

jeremylt commented Oct 20, 2023

Describe the Issue

In this PR it was mentioned that assert.isTrue() is preferred over assert() in our codebase.

This should be documented somewhere if this is our standard. Also, a lot of the curriculum tests don't use this, so it would be worth making an issue to have someone go change everywhere we missed this standard.

Affected Page

https://contribute.freecodecamp.org/#/codebase-best-practices

Your code

N/A

Expected behavior

Documentation should state standards.

Screenshots

No response

System

All

Additional context

No response

@lasjorg
Copy link
Contributor

lasjorg commented Oct 20, 2023

I wouldn't say this is specific to that, or any, assert method. If a method exists that expresses what is tested for, we should prefer it.

A lot of the assert method can be replaced with just assert(expression) but the point of using the methods is both in readability and implementation.

  1. Using the proper method makes it clear what is expected.

  2. We should prefer the internal testing library implementation.

assert(Array.isArray([]), 'empty arrays are arrays');
assert.isArray([], 'empty arrays are arrays');
  1. The test should express what is being tested for instead of relying on the assert message.

Edit: I am not suggesting we have to replace all assert(true === true) with assert.isTrue(true === true) but if a test is using that method I see no point in changing it to assert(true === true).

I don't think we have to enforce the use of isTrue but I would still prefer it to assert(expression)

@ShaunSHamilton
Copy link
Member

Some context: freeCodeCamp/freeCodeCamp#43105 (comment)

I have been advocating for using the specific assertions, because the default assertion messages can be very helpful: freeCodeCamp/freeCodeCamp#40518

We mostly1 do not use them, but I have always hoped we would in the future.

Footnotes

  1. In some of the backend challenges, we output the assertion messages to the browser console, and I have frequently used this to debug failed tests when helping Campers on the forum: https://forum.freecodecamp.org/t/help-with-issue-tracker-project/433239/3

@naomi-lgbt naomi-lgbt self-assigned this Jan 9, 2024
@a2937
Copy link
Member

a2937 commented Jan 31, 2024

Would the documentation for the assertion methods would go on this page? https://contribute.freecodecamp.org/#/how-to-work-on-coding-challenges?id=writing-tests

@Sembauke Sembauke changed the title Assert Usage Standards document assert usage standards for contributors Sep 16, 2024
@Sembauke Sembauke transferred this issue from freeCodeCamp/freeCodeCamp Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

5 participants