Tests and transform statements #16

beatthat · 2020-10-06T00:37:35Z

This is still a draft but enough here that it's worth getting some feedback.

PR adds basic test coverage adds the feature to pass an optional transform(s:Statement): Statementfunction with complete, pass, and fail

TODO:

run format check, lint, type check and tests in github actions

NOTES/QUESTIONS:

any objection to a convenience function like moveOn that sends in sequence COMPLETED, PASSED|FAILED (depending on score), and optionally TERMINATED ?
any objection to using async instead of Promise.then?
I added prettier and tried to make it conform as much as possible to the style already in place to avoid file reformats. Any objection to reducing the line length down to 80 though?

CookieCookson · 2020-10-06T20:43:24Z

Thanks for putting so much of your time into this PR - I can see you have been super busy!

I am finding it quite difficult to review this PR, it seems to be tackling lots of different issues and its not easy to differentiate all the changes that are happening.

From what I can see you have:

Added Prettier (causing an awful lot of line changes!)
Added Tests (With GitHub Actions in todo)
Added SendStatementOptions for Extension transforming

You've also raised some valid points about having another convenience function, async and line lengths.

In order for me to review these changes, would it be possible for you to split up the PR into multiple PRs that each tackle an individual issue? The less line changes the better, ideally only changing lines where new additions have been made rather than reformatting whole files. (Except where prettier rules are introduced in a prettier PR which is designed to reformat files)

Also with the points raised in your NOTES/QUESTIONS above, they would be better suited either in the separate PRs as discussion points relating to the code changes you are making (async, line lengths) or in new issues (convenience function).

I do love where these changes are heading, if we can establish some tougher lint rules it should help other people contribute in the future!

beatthat · 2020-10-06T20:46:53Z

sure. What if I just broke out prettier and did that first?

It's much easier to collaborate on code with deterministic formatting cos avoids the thing where everyone's code editor will overwrite each other's files, etc.

beatthat · 2020-10-06T21:17:08Z

prettier PR is here: #17

CookieCookson · 2020-10-07T20:18:16Z

Thanks for the prettier PR, it looks great and I'll be sure to try and bring some consistency into the other @xapi/* projects with this in the future.

Unfortunately this PR's diff hasn't updated with the new changes meaning its hard to see the new amends. Could you edit this PR to merge into a different branch and then back to master to trigger a diff refresh? Or alternatively open up a new PR with these same changes?

beatthat · 2020-10-07T20:50:14Z

yeah, i will rebase it

…

On Wed, Oct 7, 2020 at 1:18 PM Christian Cook ***@***.***> wrote: Thanks for the prettier PR, it looks great and I'll be sure to try and bring some consistency into the other @xapi/* projects with this in the future. Unfortunately this PR's diff hasn't updated with the new changes meaning its hard to see the new amends. Could you edit this PR to merge into a different branch and then back to master to trigger a diff refresh? Or alternatively open up a new PR with these same changes? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPCPOFEHXC57M4CEBAKULSJTEJPANCNFSM4SFLC4DA> .

feature: adds type check feature: test coverage for initialize sequence feature: isCmiAvailable checks if cmi5 launch params are present feature: test coverage for initialize sequence - throws on auth fail - throws on getLaunchData fail - swallows errors on getLearnerPrefs failed - throws on post INITIALIZED statement fails feature: Cmi5.complete applies optional statement transform when provided feature: Cmi5.pass applies optional statement transform when provided feature: Cmi5.pass applies optional statement transform when provided feature: Cmi5.fail posts a FAILED statement with a result score passed as a number feature: Cmi5.fail applies optional statement transform when provided improvement: test constructor parses launch params from window.location improvement: test cmi5 complete statement improvement: tests Cmi5.complete throws if launch mode not 'Normal' improvement: tests Cmi5.complete sends duration as time since initialized improvement: tests Cmi5.pass posts a PASSED statement with a result score improvement: tests Cmi5.pass posts a PASSED statement with a result score passed as a number improvement: tests Cmi5.pass posts a PASSED statement with no result score when score not passed improvement: test Cmi5.pass throws if passed score is beneath masteryScore improvement: test throws exception if PASSED invalid for launch mode improvement: test Cmi5.pass assigns an ObjectiveActivity when passed as second param improvement: test Cmi5.pass sends duration as time since initialized improvement: Cmi5.fail posts a FAILED statement with a result score improvement: test Cmi5.fail posts a FAILED statement with no result score when score not passed improvement: test tCmi5.fail wqhrows exception if FAILED invalid for launch mode improvement: test sCmi5.fail wqends duration as time since initialized improvement: test Cmi5.terminate posts a TERMINATED statement improvement: test sCmi5.terminate sends duration as time since initialized

beatthat · 2020-10-07T21:36:07Z

@CookieCookson did anything change with config relating to github actions?

github test action is failing to checkout the project at all with a weird errors:

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/tests-and-transform-statements*:refs/remotes/origin/tests-and-transform-statements* +refs/tags/tests-and-transform-statements*:refs/tags/tests-and-transform-statements*
  The process '/usr/bin/git' failed with exit code 1
  Waiting 11 seconds before trying again
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/tests-and-transform-statements*:refs/remotes/origin/tests-and-transform-statements* +refs/tags/tests-and-transform-statements*:refs/tags/tests-and-transform-statements*
  The process '/usr/bin/git' failed with exit code 1
  Waiting 16 seconds before trying again
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/tests-and-transform-statements*:refs/remotes/origin/tests-and-transform-statements* +refs/tags/tests-and-transform-statements*:refs/tags/tests-and-transform-statements*
  Error: The process '/usr/bin/git' failed with exit code 1

...use circleci pretty much everywhere else, so not a ton of experience with github actions. Googling seems to say that this might be github-downtime issue, but skeptical about that. Maybe it's something about my action config?

beatthat · 2020-10-07T21:41:59Z

tried reopen as a new branch/pr and same problem...anything change in terms of settings that might make checkout fail?

beatthat · 2020-10-07T22:12:45Z

i think the checkout fails were related to changes coming from my forked branch and then it wants to run an action in your repo (although that seemed to work ok w the prettier branch?)

Not sure yet what the correct way to set this up is. But last change I made got it working.

CookieCookson

Looks really great! Just a few minor tweaks required and we can get this merged in 👍

package.json

src/Cmi5.ts

CookieCookson · 2020-10-08T19:14:38Z

src/Cmi5.ts

-    score?: ResultScore,
-    objective?: ObjectiveActivity
+    score?: ResultScore | number,
+    objectiveOrOptions?: ObjectiveActivity | SendStatementOptions


Why is this like this? Should they not be separate parameters?

number 1 reason is backwards compatibility, and I'm assuming from the existing signature that you consider adding an objective activity common enough to make it a default param.

The thing I was trying to avoid was having a positional arg that callers would need to set null in order to use options, e.g.

cmi.pass(0.9, null, { transform: (s) => ...}

...on the other hand, you want to make it easy to do maybe a transform and add the objective activity, so what do you think of just making objectiveActivity another property in options (last commit)?

So this way callers can do:

// pass just a plain objective activity (backwards compatible) cmi.pass(0.9, { objectType: "Activity", id: "http://example.com/object/12345", definition: { type: "http://adlnet.gov/expapi/activities/objective" }, })

// pass an a objective activity as an option cmi.pass(0.9, { objectiveActivity: { objectType: "Activity", id: "http://example.com/object/12345", definition: { type: "http://adlnet.gov/expapi/activities/objective" }, }, tranform: (s) => {...s} })

Currently, backwards compatibility is not top of the priorities list. As its still in pre-release, I want to make sure we get the structure of the signatures right before announcing a v1.0.0. I am possibly thinking that the first param should always be an object... I will take a look through the other methods and see if I can think of something that is future proof and user friendly.

As I haven't had enough experience in using cmi5 in real life applications, I'm not certain how often one would need to add an objective activity. Everything that has been implemented has been based off of requirements in the cmi5 specification, and not from experience using cmi5 in the wild.

I suppose if both the objective activity and a custom transform is required, the objective activity could be added in the transformation?

Yes, at the limit, can do anything/everything with the transform, but as a convenience, I also put objectiveActivity as another property of options. In future could put other common things that just get copied somewhere in the statement as properties in the options object e.g. resultExtensions and contextExtensions

src/Cmi5.ts

beatthat · 2020-10-08T20:20:34Z

great. making those changes now. About merging: I noticed last PR got merged with all the branch commits intact, which I know is the default github option, but do you prefer it to 'SQUASH MERGE'? For most repos I work on, we prefer the squash for PRs and then disable all the other options in settings

CookieCookson · 2020-10-09T07:31:25Z

great. making those changes now. About merging: I noticed last PR got merged with all the branch commits intact, which I know is the default github option, but do you prefer it to 'SQUASH MERGE'? For most repos I work on, we prefer the squash for PRs and then disable all the other options in settings

Good shout, I completely missed that. I've configured all the repos to accept squash merges only now.

CookieCookson · 2020-10-13T20:57:13Z

Great job! I will merge in the changes into master, and get the updates to the docs drafted before doing a release (v0.2.0).

beatthat marked this pull request as ready for review October 6, 2020 00:46

beatthat marked this pull request as draft October 6, 2020 00:47

beatthat mentioned this pull request Oct 6, 2020

easier way to pass extensions and test support #7

Closed

beatthat added 3 commits October 7, 2020 14:08

add a missing newline at end of changelog

a8753e2

add name to checkout job in github actions

9361593

add debug settings for github-action checkout

52eda34

beatthat added 2 commits October 7, 2020 14:50

try different format for action config

a20264c

try w no

70a0849

removes debug from github checkout action

7c06e01

CookieCookson requested changes Oct 8, 2020

View reviewed changes

beatthat added 4 commits October 8, 2020 13:29

moves jest config to its own file

3ceb04c

improvement: run all tests in ci

c749800

makes all options params optional

8437142

feature: Cmi5.pass assigned objective activity passed through options

61704db

beatthat requested a review from CookieCookson October 13, 2020 18:11

beatthat marked this pull request as ready for review October 13, 2020 18:24

CookieCookson approved these changes Oct 13, 2020

View reviewed changes

CookieCookson linked an issue Oct 13, 2020 that may be closed by this pull request

easier way to pass extensions and test support #7

Closed

CookieCookson merged commit c1d0541 into xapijs:master Oct 13, 2020

beatthat deleted the tests-and-transform-statements branch October 15, 2020 20:19

beatthat restored the tests-and-transform-statements branch October 15, 2020 20:19

beatthat deleted the tests-and-transform-statements branch October 15, 2020 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests and transform statements #16

Tests and transform statements #16

beatthat commented Oct 6, 2020 •

edited

Loading

CookieCookson commented Oct 6, 2020

beatthat commented Oct 6, 2020

beatthat commented Oct 6, 2020

CookieCookson commented Oct 7, 2020

beatthat commented Oct 7, 2020 via email

beatthat commented Oct 7, 2020

beatthat commented Oct 7, 2020 •

edited

Loading

beatthat commented Oct 7, 2020 •

edited

Loading

CookieCookson left a comment

CookieCookson Oct 8, 2020

beatthat Oct 8, 2020 •

edited

Loading

CookieCookson Oct 9, 2020

CookieCookson Oct 11, 2020

beatthat Oct 11, 2020

beatthat commented Oct 8, 2020

CookieCookson commented Oct 9, 2020

CookieCookson commented Oct 13, 2020

Tests and transform statements #16

Tests and transform statements #16

Conversation

beatthat commented Oct 6, 2020 • edited Loading

CookieCookson commented Oct 6, 2020

beatthat commented Oct 6, 2020

beatthat commented Oct 6, 2020

CookieCookson commented Oct 7, 2020

beatthat commented Oct 7, 2020 via email

beatthat commented Oct 7, 2020

beatthat commented Oct 7, 2020 • edited Loading

beatthat commented Oct 7, 2020 • edited Loading

CookieCookson left a comment

Choose a reason for hiding this comment

CookieCookson Oct 8, 2020

Choose a reason for hiding this comment

beatthat Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

CookieCookson Oct 9, 2020

Choose a reason for hiding this comment

CookieCookson Oct 11, 2020

Choose a reason for hiding this comment

beatthat Oct 11, 2020

Choose a reason for hiding this comment

beatthat commented Oct 8, 2020

CookieCookson commented Oct 9, 2020

CookieCookson commented Oct 13, 2020

beatthat commented Oct 6, 2020 •

edited

Loading

beatthat commented Oct 7, 2020 •

edited

Loading

beatthat commented Oct 7, 2020 •

edited

Loading

beatthat Oct 8, 2020 •

edited

Loading