Fix HED error reporting and tests

[happy5214]

This PR fixes the reporting of IssueError objects thrown from hed-validator by converting them into proper Issue objects. It also cleans up several errors in the HED tests that should have been failing. This should help with the eventual merging of #2167.

Fixes #2170

[happy5214]

Let me know if the additional warnings for missing HEDVersion fields are a problem.

[effigies]

This is now reliably timing out tests at 10s. Looking at it, I would guess that there's an efficiency problem with the use of issues.push. #2162 removed some uses to avoid call stack limits.

[happy5214]

This is now reliably timing out tests at 10s. Looking at it, I would guess that there's an efficiency problem with the use of issues.push. #2162 removed some uses to avoid call stack limits.

I didn't really add any new uses of push other than catch handlers (which shouldn't be executing that much anyway). I also eliminated a function call. I tried a flatMap, which didn't help. Any other ideas?

[effigies]

Nope. @mguaypaq Any advice? Or @rwblair?

[mguaypaq]

Would it be possible to run the tests with profiling enabled? Even better would be having both "before this PR" and "after this PR" profiles, to see any significant changes. Without something like this, it's really hard to diagnose performance issues.

[effigies]

It does not look trivial, but we can look into it. If there's an example somewhere of someone setting up profiling for jest tests, that would be a helpful model.

[happy5214]

The timeouts are because the tests are not structured properly. Any time a testcase assertion fails, the isdone callback is never called, which leads to the timeout (as Jest never figures out the block is finished). Just fix the assertions in bids.spec.js and you're good to go.

[codecov]

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.43%. Comparing base (f40d4de) to head (a656a9c).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
bids-validator/validators/hed.js	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2174      +/-   ##
==========================================
+ Coverage   85.72%   87.43%   +1.70%     
==========================================
  Files          91      133      +42     
  Lines        3784     7045    +3261     
  Branches     1221     1671     +450     
==========================================
+ Hits         3244     6160    +2916     
- Misses        454      789     +335     
- Partials       86       96      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[effigies]

Okay, so it looks like datasets without HED in are now expected to raise HED warnings?

[happy5214]

I rewrote the branch so that it no longer raises warnings for missing HEDVersion fields except when there's HED data. In doing so, I also (accidentally) clobbered the isdone changes (which really should be implemented instead by making the anonymous function an async).

[effigies]

Thanks!