OpenBCca / openbc-web

https://openbcca.github.io/openbc-web/
1 stars 2 forks source link

Jest integration and initial tests for Header #11

Closed nam-m closed 11 months ago

nam-m commented 11 months ago

Summary of changes

Notes

SamHuo213 commented 11 months ago

Please merge from main.

SamHuo213 commented 11 months ago

Thanks for your PR,

I have the following comments:

1) jest.config.mjs, are all the comments necessary? 2) Can jest.setup.js be a ts file? What is this file for? 3) What is clsx package? 4) What is the intention of tests folder? 5) is header.snapshot.tsx intended for demo? 6) is index.test.ts intended for demo? 7) I think it should say 'should render header' and describe is the component name and/or function name, on error the print out will be 'header should render header' 8) are the href necessary?

nam-m commented 11 months ago

@SamHuo213 thanks for reviewing, here are my answers

1. jest.config.mjs, are all the comments necessary?

I think they're helpful to me, maybe i can summarize them better? The comments are from the template in Nextjs setup guide

2. Can jest.setup.js be a ts file? What is this file for?

I created during a guide for setup using Babel, but we're not using Babel so I will remove this

3. What is clsx package?

I was testing sth with it, but didn't need anymore. I will remove then

4. What is the intention of **tests** folder?

We can decide on whether to keep the tests here or in each component folder. I guess this is up to discussion

5. is header.snapshot.tsx intended for demo?

Yes, this snapshot is for developers to compare their snapshots to the one on main repo

6. is index.test.ts intended for demo?

Yes, but I will remove this as there are other demo tests

7. I think it should say 'should render header' and describe is the component name and/or function name, on error the print out will be 'header should render header'

Good point, will change

8. are the href necessary?

href is needed to test toHaveAttribute in Header.test.tsx, I think this will be useful to test the navigation links later

nam-m commented 11 months ago

@SamHuo213 I have made changes to the code, so it can be reviewed again now

SamHuo213 commented 11 months ago

I think they're helpful to me, maybe i can summarize them better? The comments are from the template in Nextjs setup guide

I subscribe to self documenting code: https://swimm.io/learn/documentation-tools/tips-for-creating-self-documenting-code

Also these conventions are nice: https://gist.github.com/anichitiandreea/e1d466022d772ea22db56399a7af576b

I ask if these comments are really necessary but I am ok with leaving them :)

I created during a guide for setup using Babel, but we're not using Babel so I will remove this

Thanks, let's keep things clean.

I was testing sth with it, but didn't need anymore. I will remove then

Thanks, let's keep things clean.

We can decide on whether to keep the tests here or in each component folder. I guess this is up to discussion

I am familiar with two conventions:

1) In the same folder. This is typical of JS projects/frameworks 2) Separate Test section with folder structure that mirrors existing structure. This is usually in C# frameworks.

Either works for me, but we should get buy in for our prefer convention.

Yes, this snapshot is for developers to compare their snapshots to the one on main repo

Ok

Yes, but I will remove this as there are other demo tests

Ok

Good point, will change

Ok

href is needed to test toHaveAttribute in Header.test.tsx, I think this will be useful to test the navigation links later

Ok

Usually I prefer not having production code that is only intended for testing but in this case I think it should be ok.

nam-m commented 11 months ago

@umsu2 @SamHuo213 I have done the following:

It took me some time going through all the changes in package-lock.json to include/exclude packages in vscode, wondering if there's a quick way to do this?

SamHuo213 commented 11 months ago

@umsu2 @SamHuo213 I have done the following:

  • Merged latest main
  • Added semicolon to test files
  • Installed testing packages in package.json and combined package-lock.json
  • Updated header & header snapshot tests

It took me some time going through all the changes in package-lock.json to include/exclude packages in vscode, wondering if there's a quick way to do this?

package-lock.json is a autogenerated file. The best way was to delete it and do an npm install.

SamHuo213 commented 11 months ago

@umsu2 @SamHuo213 I have done the following:

  • Merged latest main
  • Added semicolon to test files
  • Installed testing packages in package.json and combined package-lock.json
  • Updated header & header snapshot tests

It took me some time going through all the changes in package-lock.json to include/exclude packages in vscode, wondering if there's a quick way to do this?

package-lock.json is a autogenerated file. The best way was to delete it and do an npm install.

@nam-m , can you please make sure to do this please :).

nam-m commented 11 months ago

@umsu2 @SamHuo213 I have done the following:

  • Merged latest main
  • Added semicolon to test files
  • Installed testing packages in package.json and combined package-lock.json
  • Updated header & header snapshot tests

It took me some time going through all the changes in package-lock.json to include/exclude packages in vscode, wondering if there's a quick way to do this?

package-lock.json is a autogenerated file. The best way was to delete it and do an npm install.

@nam-m , can you please make sure to do this please :).

done