search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
296 stars 112 forks source link

Use the Jest testing framework and switch to GitHub Actions #254

Closed richard-viney closed 2 years ago

richard-viney commented 2 years ago

Thanks for the work you've done on this library!

I've recently encountered some DICOMs that have issues reading & writing in dcmjs due to what appears to be some bugs in parsing and multi frame serialisation, and plan to open a couple PRs for these with fixes. However I noticed initially that there was no test runner for the current tests, and so added Jest as it's a common choice these days and makes writing additional best-practice tests more straightforward.

The main changes in this PR are:

Thoughts on adopting Jest as a test runner?

pieper commented 2 years ago

Thanks for the contribution 👍

I don't know much about testing frameworks, which is why there was just a silly stub so far, so this will be a big improvement. I did have one suggestion for vitest but if you have a preference for jest we can go with that.

In general it looks good - is it ready for review/merge?

richard-viney commented 2 years ago

Vitest is fairly new and looks promising, though perhaps less relevant for a non-frontend project such as dcmjs. I actually use Vite itself (but not vitest) every day and it's great.

Jest is the most commonly used JS testing framework by some distance, and has a 93% satisfaction ranking here: https://2021.stateofjs.com/en-US/libraries/testing. Hence is a reasonable choice, and anyone wanting to contribute to dcmjs will likely be at home with it too.

If there's a desire to adopt vitest in future (its runner is a little bit nicer than Jest in some ways) then the transition would be mainly changing configs and adding some imports, because vitest supports the same describe/it/expect syntax as Jest so there'd be essentially no code changes.

Yes this PR is merge-able right now, I've reviewed and tested it all locally.

pieper commented 2 years ago

This looks good to me, but the circleci test isn't working now and it's blocking the merge. I could override it for now, but we'd want to have it working going forward. @richard-viney do you have any ideas about that?

richard-viney commented 2 years ago

I've pushed a couple of updates to the CircleCI config, but can't actually see any logs from the CircleCI runs .. are they available?

If you like I could switch it to use GitHub Actions, might be simpler having it all in one place?

pieper commented 2 years ago

@richard-viney Yes, I would really like to see us switch to github actions and improve the testing. Can you add that to this PR?

richard-viney commented 2 years ago

The conversion to GitHub Actions is done. The new workflows need to be approved first before they will actually run.

The NPM_TOKEN needs to be setup as a GitHub secret by a repo admin. The CircleCI integration hook also needs to be removed by an admin.

I haven't been able to directly test the release workflow, but may be best to just merge and see what, if anything, breaks. There might be a permissions issue, I'm not 100% certain.

richard-viney commented 2 years ago

I've added a set of recommended extensions, including one for the firsttris.vscode-jest-runner VS Code extension that lets you really easily run individual tests and set breakpoints in the IDE.

pieper commented 2 years ago

@richard-viney thanks for your work on this. Since it looks like you are still committing and this hasn't been reviewed yet I changed to draft for now. Let us know when you think it's ready to go and we can go through setting up the keys and everything. I haven't had a chance to look into any of that.

richard-viney commented 2 years ago

This is ready to go. I've squashed it into two commits: one that adds Jest and one that switches to GitHub Actions.

To setup the secret for GitHub Actions (which would be best done prior to merging):

Please let me know if there's anything else I can do.

richard-viney commented 2 years ago

Please let me know if I can help with the review/merge process any further here. I've got a couple of bug fixes to submit along with new-style tests for them that would be great to get the ball rolling on as well :-).

pieper commented 2 years ago

@igoroctaviano said he can take a look at this since he has done similar work on other projects.

pieper commented 2 years ago

Thanks for the contribution @richard-viney 👍 I just want another pair of eyes since it's not something I have direct experience with.

igoroctaviano commented 2 years ago

@richard-viney publish works (tested in a separate fork). Can you please resolve the conflicts before we can move forward?

richard-viney commented 2 years ago

Great re. publish - conflicts are resolved and rebased onto master.

Is there anything outstanding?

pieper commented 2 years ago

This looks good, but it's not clear to me how to interpret the test results. It looks like lots of error messages but then the tests are listed as passing.

https://github.com/dcmjs-org/dcmjs/runs/6097327896?check_suite_focus=true

@igoroctaviano should we go ahead and merge and then remove the circleci from the required checks.

richard-viney commented 2 years ago

AFAICT the console.error() calls haven't changed and happen identically on current master. However, Jest takes the extra step/precaution of printing a full stack trace for a console.error() call, which does blow up the test logs a bit. The tests themselves could definitely be tidied up somewhat, but I think that's outside the scope of this PR which is for switching to Jest and GitHub Actions.

One of my followup PRs is a fix for the Invalid vr type ... error that is caused by problems in the BinaryRepresentation class.

pieper commented 2 years ago

Thanks for the explanation 👍 I'll go ahead and merge and we can sort out what to do next.

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 0.20.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: