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

added tests for randomString and getPusher #2181

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

saminarp
Copy link
Contributor

Description

I have written unit tests for the client/app/utils. The coverage report for the index.js has gone up to 65% with the corresponding tests I have written in index.test.js

More Details

While I was able to cover all the lines for the function randomString, the functions setCsrfToken, getPusher uses DOM implementation which makes it very tricky to mock it in Jest. Thus, they remain uncovered.

Corresponding Issue

#2128

Screenshots

CleanShot 2022-10-19 at 03 55 51@2x


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

@saminarp saminarp requested a review from julianguyen October 19, 2022 07:57
Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Great work so far! Thanks for taking this on.

Looks like there's some ESLint failures in the CI build. You'll want to fix them. I recommend running yarn lint:eslint locally to reproduce the failures and verify when you've fixed them.

import { JSDOM } from 'jsdom';


describe('randomString, setCsrfToken, getPusher tests ', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think a better way to organize this is to structure the tests like so:

describe('Utils', () => {
  describe('randomString()`, () => {
    // etc.
  });
  
  describe('setCsrfToken()`, () => {
    // etc.
  });
  
  describe('getPusher()`, () => {
    // etc.
  });
  
   describe('renderContent()`, () => {
    // etc.
  });
});

});


it('should be equal to pusher key and cluster when getPusher is called', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll also want to test the return null case as well, when window.pusher does into exist.

@saminarp saminarp marked this pull request as draft October 22, 2022 06:11
@saminarp
Copy link
Contributor Author

Hi @julianguyen I've done the changes, please review ☺️

@saminarp saminarp marked this pull request as ready for review October 22, 2022 07:46
Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, the tests looks good! 🎉

Just a few comments!

+ Math.random()
.toString(36)
.substring(2, 15);
export const randomString = (): string => Math.random().toString(36).substring(2, 15)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to export these functions individually in order to be able to test them. You should be able to import the Utils object and call the functions as stated in the previous comment.

@@ -0,0 +1,21 @@
const { randomString, getPusher } = require('../index');
Copy link
Member

Choose a reason for hiding this comment

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

Let's use imports here instead, I recommend just pulling in the Utils object instead of adding new exports for each function.

Suggested change
const { randomString, getPusher } = require('../index');
import Utils from '../index';

Then you can call Utils.randomString() and Utils.getPusher().

@saminarp saminarp marked this pull request as draft October 24, 2022 06:50
@saminarp saminarp marked this pull request as ready for review October 24, 2022 07:01
Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for working on this 🎊

@julianguyen julianguyen merged commit 22aa53d into ifmeorg:main Oct 24, 2022
@welcome
Copy link

welcome bot commented Oct 24, 2022

Thank you for merging this pull request with us! If you haven't already, in another pull request, please add yourself to our About page.

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.

2 participants