Refactor and test [github] token persistence #1863
Conversation
Generated by 🚫 dangerJS |
| clock = sinon.useFakeTimers(new Date(2017, 9, 15).getTime()) | ||
| }) | ||
| afterEach(function() { | ||
| clock.restore() |
There was a problem hiding this comment.
This was causing a timeout in the new tests.
| this.save = undefined | ||
| } | ||
|
|
||
| async initialize() { |
There was a problem hiding this comment.
This code is a bit difficult to understand, in part because it's working against the interface of json-autosave which wants to own the data structure.
There was a problem hiding this comment.
Not trivial indeed, but I think I get the gist of it. 👍
paulmelnikow
changed the title
|
This is ready for review! |
| this.save = undefined | ||
| } | ||
|
|
||
| async initialize() { |
There was a problem hiding this comment.
Not trivial indeed, but I think I get the gist of it. 👍
|
|
||
| let path, persistence | ||
| beforeEach(function() { | ||
| path = tmp.tmpNameSync() |
There was a problem hiding this comment.
I'm guessing tmp will automatically clean up on exit?
There was a problem hiding this comment.
Huh, it's not totally clear from the docs. Since we're not writing anything sensitive and it's a fresh name every time, I don't feel it's important to clean up the files, though I could be persuaded.
https://www.npmjs.com/package/tmp#synchronous-filename-generation
There was a problem hiding this comment.
It does say the following in the options section: "keep: signals that the temporary file or directory should not be deleted on exit, default is false". I'm generally not a fan of tests leaving droppings behind them, but we're probably fine here according to the documentation!
|
Before I merge this I want to be ready to deploy it. There's something wrong with the admin endpoint; so I can't back up the tokens. I'm going to fix that first. Added: Ref #1886 |
|
Okay, #1886 deployed and tokens backed up. Going to merge and deploy this now! |
After #1861 is merged I will get this into working shape.
Ref #1848