refactor: Extract statement construction functions, use async/await, begin extracting tests.
#274
Conversation
emmanuel
force-pushed
the
feat/use-async-await
branch
from
8eb2d97 to
be8756c
Compare
|
@emmanuel This is a really helpful contribution, thank you! If you get it rebased I can give it a more thorough review next week, at first glance it is a nice refactor and test addition. |
Much of the variation in testing can be handled by validating statement construction. Almost all of it, really, with the notable exceptions of `initialize()` and `moveOn()`. Anyhow, I intend to extract the remaining statement tests from the `Cmi5` class tests (`cmi5.spec.ts`), and to extend the statement tests to cover more cases.
emmanuel
force-pushed
the
feat/use-async-await
branch
from
be8756c to
27134f8
Compare
|
@CookieCookson Great! I'm glad this looks like a useful contribution. And thanks for the encouragement! I rebased and tests still pass locally. |
|
Actually, regarding the tests: I dropped the mocking of time in the statement tests, which mostly works, but occasionally fails. IOW, I introduced a flaky test. I have a thought about how to address that (my intention is to do so without returning to mocking), but it's not implemented here. Right now, if you get a test failure on the duration tests like this: That's the flake. Re-run the tests (maybe more than once) and it will pass. I've only seen the failure locally (not in CI), but it could happen in CI, too. |
|
Hi @CookieCookson, how's it going? Have you had a chance to review this? Is there anything I can do to help? |
|
@emmanuel I've not had any time again, sorry. I will see if I can get some time this week to investigate the flaky test(s). I can see it just being from slow execution time or a rounding error. |
|
To be clear, I wasn’t expecting you to investigate the test I introduced! I imagined merging the flaky test and hitting retry on CI if it fails there. Not the cleanest, perhaps, but addressing it properly may be a significant step up in complexity. That said, I didn’t realize that’s what you’re waiting on. If so, let me take another pass and resolve the intermittent fail on the XAPI duration.
|
|
@CookieCookson alright, I resolved the flake. In lieu of mocking, stubbing, or adding a dependency (e.g., |
|
I'd be delighted to see this land soon. Please let me know if there's anything else you'd like to see before merging this. |
|
Excellent and creative resolution - great job! I should have time this weekend to get this reviewed and released. Did you see my comment on the other closed PR? I'm concerned about how you may actually consume the changes made as it doesn't seem that anything new is being exported. |
|
@CookieCookson thanks for looking again. I looked back at the last PR ("support Cmi5 from the backend"), and I think you're referring to this comment, and yet ...I'm confused 🤔. The So... to what were you referring? |
|
I was thinking that with needing to use this on the backend, you will need to install the package and import the Abstract CMI class. When this package builds, it uses the original Cmi5 class as the entry point so you will be forced to use the class that uses the browser APIs which you may be trying to avoid. I may be confused here too, it would be good to have a code example of how you plan on consuming the Abstract CMI class in another project. |
|
@emmanuel I finally found some time to give this the time it needs! The code change was substantial so it took a fair while to review but its lots of good stuff. All merged, this plus your previous contributions plus some security vulnerability patches have been released into A huge thank you for your contributions on this project, and an apology that its taken months and months to get these code changes out into the wild. Is #265 now ready to be closed up or are there additional code changes still required for this? |
|
I went ahead and close #265; all done there! Thanks for your encouragement and review. And again, thanks for publishing this project. |
Main thing here is extracting statement construction from other logic.
First goal is to support simpler and more extensive testing of statement construction (see
cmi5-statements.spec.ts), but secondarily it opens up the library to additional (unforeseen) use-cases by providing CMI5-compliant xAPI statement construction ala carte from the LRS client. If you're into patterns, this PR could be described as an application of the "single responsibility principle" pattern (statement construction vs LRS communication).Stylistically, this de-emphasizes classes and focuses on functions and data types.