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

🧹 Enketo cleanup #1113

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Abby-Wheelis
Copy link
Member

Addressing comments from #1063 and the codecov report for affected files

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c7e9244) 77.55% compared to head (f0a55f5) 79.39%.
Report is 40 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
+ Coverage   77.55%   79.39%   +1.84%     
==========================================
  Files          28       28              
  Lines        1702     1699       -3     
  Branches      367      364       -3     
==========================================
+ Hits         1320     1349      +29     
+ Misses        382      350      -32     
Flag Coverage Δ
unit 79.39% <100.00%> (+1.84%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
www/js/survey/enketo/enketoHelper.ts 83.62% <100.00%> (+13.79%) ⬆️

... and 3 files with indirect coverage changes

Abby Wheelis added 2 commits December 4, 2023 16:46
one of my main issues was that I needed to await where I was checking for what something resolves to, ".resolves.to..." was not enough

I then fixed some errors that popped up once I was testing properly, and added a few more cases to some of the tests to cover the gaps
@Abby-Wheelis
Copy link
Member Author

I have added some more tests, and now the testing coverage is up to about 98% of the lines for enketoHelper.ts. This gap is accounted for by not having a test cases that tests out the xml->JSON transformer. @JGreenlee do you think we'd be able to test this easily? I tried briefly, but got an error from the transformer and wasn't sure what the best approach would be.

I think I've also addressed the comments from #1063 except for the couple of places where I was confused. You can see those review comments here: 1063 review

@Abby-Wheelis
Copy link
Member Author

Simulator Screenshot - iPhone 13 Pro - 2023-12-05 at 14 10 20

Double checked, working well in the emulator!

Pending my confusion over review comments on the previous PR, I think this is ready to start review!

@Abby-Wheelis Abby-Wheelis marked this pull request as ready for review December 5, 2023 21:12
JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this pull request Dec 15, 2023
To be more consistent with the rest of the codebase, as a rule of thumb we're having top-level functions use 'function' synax while one-liners or nested functions are arrow functions.

Also, resetStoredConfig was renamed to _test_resetStoredConfig because that change is coming anyway in e-mission#1113 and this will make it easier to resolve merge conflicts
@Abby-Wheelis Abby-Wheelis changed the base branch from service_rewrite_2023 to master February 8, 2024 00:24
@Abby-Wheelis
Copy link
Member Author

Status on this PR:

I think I have managed to resolve the mess that the merge conflicts created, but I still have several tests failing (I commented them out for now, and the coverage on the file is around 80%):

  1. testing the case where invalid timestamps are entered is resolving with the whole form instead of rejecting with the expected error message, I haven't figured out why yet
  2. the test for loadPreviousResponseForSurvey is resolving with undefined instead of the expected object, I don't know why yet
  3. the fetchSurvey test is also returning undefined, but I don't know why

Prettier is also failing, even though I am running prettier on the command line to try and clean up the code.

@Abby-Wheelis
Copy link
Member Author

testing the case where invalid timestamps are entered is resolving with the whole form instead of rejecting with the expected error message, I haven't figured out why yet

The Promise is not rejecting because the resolvedTimestamps turn out to be undefined and the call to Promise.reject seems to only really apply to the resolveTimestamps function, rather than the parent Promise, which is fairly complicated. I can see in the console that the error is thrown, but I can't figure out how to keep the Promise from resolving, which I think is the expected behavior, since we want to reject invalid timestamps...

@Abby-Wheelis
Copy link
Member Author

the test for loadPreviousResponseForSurvey is resolving with undefined instead of the expected object, I don't know why yet

The issue here is that there are no responses fetched, I'll have to check on the mocks to figure out why there are no responses, starting with looking at what might have changed since this test was running previously...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Issues being worked on
Development

Successfully merging this pull request may close these issues.

2 participants