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 bug in assertions returning true for some tests #100

Merged
merged 31 commits into from
Apr 9, 2024

Conversation

galadim1
Copy link
Contributor

@galadim1 galadim1 commented Apr 5, 2024

Previous issues
Most of the existing tests were written incorrectly.
The assertions are inside a promise which does not return. The test will always return true.

Improvements Done

  1. All tests which does not return anything were rewritten
  2. Await/async used instead of done/done() for deprecation error tests

How to properly test
To properly test;

  1. Clone the repo
  2. Change any assertion
  3. Run the test
  4. The test should fail e.g

For more context, here is the shortcut ticket
https://app.shortcut.com/chartmogul/story/57880/rewrite-the-assertion-in-chartmogul-node-library

Copy link

This pull request has been linked to Shortcut Story #57880: Rewrite the assertion in Chartmogul Node library.

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@kamilpavlicko kamilpavlicko left a comment

Choose a reason for hiding this comment

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

LGTM, great refactoring

Copy link
Contributor

@whobubble whobubble left a comment

Choose a reason for hiding this comment

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

Test suite works (Tested locally)
package.json formatting can be reverted

package.json Outdated Show resolved Hide resolved
@galadim1 galadim1 merged commit 7acc23e into main Apr 9, 2024
4 checks passed
@galadim1 galadim1 deleted the Fix-bug-in-assertions-returning-true-for-some-tests branch April 9, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants