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

chore(site): Add unit tests, mocks #514

Merged
merged 12 commits into from
Mar 23, 2022
Merged

chore(site): Add unit tests, mocks #514

merged 12 commits into from
Mar 23, 2022

Conversation

presleyp
Copy link
Contributor

This addresses part of #485.

  • I extracted redirect functions and unit tested them. I moved them into utils so we can reuse them elsewhere if needed.
  • In my attempt to test the login/redirect flow, I made some improvements to the testing setup (mocked more endpoints, moved the Router out of App so you can use a test router with it, made the mock entities have consistently named ids). But ultimately the test is not working and is taking longer than it's worth to figure out why, so I'm going to come back to it when we're further along.
  • I happened across the fact that we had placeholders instead of labels in our sign in form and changed that for accessibility reasons. I'd like the tests to select the inputs by the labels but that also wasn't working.

@presleyp presleyp requested a review from a team as a code owner March 21, 2022 23:34
@presleyp presleyp self-assigned this Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #514 (a0eca97) into main (26d24f4) will decrease coverage by 0.13%.
The diff coverage is 28.20%.

@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
- Coverage   60.35%   60.22%   -0.14%     
==========================================
  Files         191      193       +2     
  Lines       10864    10875      +11     
  Branches       85       83       -2     
==========================================
- Hits         6557     6549       -8     
- Misses       3587     3603      +16     
- Partials      720      723       +3     
Flag Coverage Δ
unittest-go-macos-latest 56.23% <ø> (+0.11%) ⬆️
unittest-go-ubuntu-latest 59.75% <ø> (-0.09%) ⬇️
unittest-go-windows-2022 55.94% <ø> (+0.03%) ⬆️
unittest-js 62.62% <28.20%> (-0.57%) ⬇️
Impacted Files Coverage Δ
site/src/AppRouter.tsx 0.00% <0.00%> (ø)
site/src/api/index.ts 69.23% <ø> (+10.40%) ⬆️
site/src/app.tsx 0.00% <0.00%> (ø)
site/src/components/Page/RequireAuth.tsx 0.00% <0.00%> (ø)
site/src/components/SignIn/SignInForm.tsx 100.00% <ø> (ø)
site/src/test_helpers/handlers.ts 21.73% <0.00%> (-16.73%) ⬇️
site/src/pages/login.tsx 90.47% <100.00%> (+2.47%) ⬆️
site/src/test_helpers/entities.ts 100.00% <100.00%> (ø)
site/src/util/redirect.ts 100.00% <100.00%> (ø)
cli/config/file.go 67.64% <0.00%> (-8.83%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26d24f4...a0eca97. Read the comment docs.

Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

Change looks good to me, thanks for fixing the login page labels and improving the test setup. The questions I had were mostly related to the semantics in the mock rest responses, otherwise ✔️

Comment on lines 13 to 15
<Router>
<App />
</Router>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud (non-blocking): Something that we lose/was nice in the prior implementation is a single entrypoint into the React application <App>. I think this PR is a step in the right direction and an improvement, so I don't think we need to optimize for this now, but I'd expect to see one component here eventually. I think a related problem is that we don't keep App thin right now, and can probably extract out the providers like in v1.

We can think on this later; I'm getting the thoughts out there, but don't want to block!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think the better way to do what I was trying to do is to split App into two components, one that has the router and providers, and one that has the routes. That way we could test the routes one with a test setup and keep Main simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did that, what do you think? let me know if there's a better name!

site/src/components/SignIn/SignInForm.tsx Show resolved Hide resolved
site/src/test_helpers/entities.ts Outdated Show resolved Hide resolved
site/src/test_helpers/handlers.ts Outdated Show resolved Hide resolved
site/src/test_helpers/handlers.ts Show resolved Hide resolved
site/src/test_helpers/handlers.ts Outdated Show resolved Hide resolved
site/src/test_helpers/handlers.ts Show resolved Hide resolved
site/src/util/redirect.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

I'm not sure if there's more work to do in the API mocks, but everything is looking good and those will be continually evolving as we go.

Great stuff!

site/src/app.tsx Show resolved Hide resolved
@presleyp presleyp merged commit f2ac81c into main Mar 23, 2022
@presleyp presleyp deleted the 465/presleyp/routing branch March 23, 2022 14:09
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants