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

test: add tests for checkAuth.ts #149

Merged
merged 3 commits into from May 29, 2023
Merged

Conversation

diivi
Copy link
Collaborator

@diivi diivi commented May 27, 2023

Adds tests for checkAuthentication.ts

@Anush008 can you check if everything still works? I mean your login and logout logic (and related stuff), I was able to log in and log out, but maybe I missed something.

I had to refactor checkAuthentication() to make it more pure and testable. Will cover more lines soon.

src/worker/background.ts Outdated Show resolved Hide resolved
@diivi diivi mentioned this pull request May 27, 2023
6 tasks
@Anush008
Copy link
Member

@diivi, the function signature has become complicated. We don't really need to tersely parameterize a relatively simple function for the sake of adding unit tests to it.

@diivi
Copy link
Collaborator Author

diivi commented May 28, 2023

imo it's still pretty simple to understand with the naming, and these side effect inducing functions should definitely be tested (no other way to test them).

If it doesn't get called frequently, it shouldn't matter as much.

@Anush008
Copy link
Member

Anush008 commented May 28, 2023

About the tests. We are just mocking values in the tests, which effectively just checks if the conditional statements work.
If we change the the implementation of the function in the future the tests will need an update too.

I don't think the test justifies changing the function signature to parameterize all the utility functions used within.

Which also means that we can't just import and use checkAuthentication() but also have to import all the utility functions that are to be passed as args whenever calling it, when all those utility functions can be captured by the function definition's closure implicitly like it is currently doing.

@diivi
Copy link
Collaborator Author

diivi commented May 28, 2023

If we change the the implementation of the function in the future the tests will need an update too.

That's how tests work everywhere😅.

Which also means that we can't just import and use checkAuthentication() but also have to import all the utility functions that are to be passed as args whenever calling it, when all those utility functions can be captured by the function definition's closure implicitly like it is currently doing.

It actually makes it purer and importing some extra functions does less harm than using foreign side effect inducing APIs.

Also without importing those functions this can't be tested, ref test parameterization.

If we need more coverage, especially with these files we need to parameterize functions.

@diivi
Copy link
Collaborator Author

diivi commented May 28, 2023

Also, the second signature is more of a "signature" since it tells you all the dependencies of the function.

@diivi
Copy link
Collaborator Author

diivi commented May 28, 2023

We are just mocking values in the tests, which effectively just checks if the conditional statements work.

That's the only thing we can test, by mocking external APIs. It's just a check for future changes as you mentioned, if something changes in the checkAuth function, it needs to pass the previous tests so devs know atleast the flow is correct, and add/edit tests for any change that they made.

@Anush008
Copy link
Member

That sounds right. 👍👍.

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

LGTM

@bdougie bdougie merged commit 3a79fac into beta May 29, 2023
6 checks passed
github-actions bot pushed a commit that referenced this pull request May 29, 2023
## [1.3.1-beta.3](v1.3.1-beta.2...v1.3.1-beta.3) (2023-05-29)

### ✅ Tests

* add tests for checkAuth.ts ([#149](#149)) ([3a79fac](3a79fac))
@github-actions
Copy link

🎉 This PR is included in version 1.3.1-beta.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@bdougie bdougie mentioned this pull request Jun 3, 2023
2 tasks
@Anush008 Anush008 deleted the test/test-checkAuthentication.ts branch June 3, 2023 02:46
github-actions bot pushed a commit that referenced this pull request Jun 3, 2023
## [1.4.0](v1.3.0...v1.4.0) (2023-06-03)

### ✅ Tests

* add tests for checkAuth.ts ([#149](#149)) ([3a79fac](3a79fac))
* add tests for utils/urlMatchers.ts ([#143](#143)) ([26702e6](26702e6))

### 🐛 Bug Fixes

* Post a highlight ([#101](#101)) ([e3b7051](e3b7051))
* remove auto-take message from triage.yml ([258a828](258a828))
* rename triage.yml ([3b9a14b](3b9a14b))
* undefined config during build ([#158](#158)) ([c986086](c986086))

### 🍕 Features

* latest highlights ([#154](#154)) ([0ccd0e7](0ccd0e7))
* uses username on highlight instead of full name ([#162](#162)) ([801fe5a](801fe5a))
@github-actions
Copy link

github-actions bot commented Jun 3, 2023

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

zer0and1 pushed a commit to zer0and1/open-sauced.ai that referenced this pull request Jul 26, 2023
## [1.3.1-beta.3](open-sauced/ai@v1.3.1-beta.2...v1.3.1-beta.3) (2023-05-29)

### ✅ Tests

* add tests for checkAuth.ts ([#149](open-sauced/ai#149)) ([3a79fac](open-sauced/ai@3a79fac))
zer0and1 pushed a commit to zer0and1/open-sauced.ai that referenced this pull request Jul 26, 2023
## [1.4.0](open-sauced/ai@v1.3.0...v1.4.0) (2023-06-03)

### ✅ Tests

* add tests for checkAuth.ts ([#149](open-sauced/ai#149)) ([3a79fac](open-sauced/ai@3a79fac))
* add tests for utils/urlMatchers.ts ([#143](open-sauced/ai#143)) ([26702e6](open-sauced/ai@26702e6))

### 🐛 Bug Fixes

* Post a highlight ([#101](open-sauced/ai#101)) ([e3b7051](open-sauced/ai@e3b7051))
* remove auto-take message from triage.yml ([258a828](open-sauced/ai@258a828))
* rename triage.yml ([3b9a14b](open-sauced/ai@3b9a14b))
* undefined config during build ([#158](open-sauced/ai#158)) ([c986086](open-sauced/ai@c986086))

### 🍕 Features

* latest highlights ([#154](open-sauced/ai#154)) ([0ccd0e7](open-sauced/ai@0ccd0e7))
* uses username on highlight instead of full name ([#162](open-sauced/ai#162)) ([801fe5a](open-sauced/ai@801fe5a))
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.

None yet

3 participants