Use randomly generated delimiter for multiline env variables #26
Conversation
SaschaMann
requested review from
DilumAluthge,
christopher-dG and
ericphanson
The previous discussion in #6 finally has a proper answer, this updates it to follow `@actions/core`: https://github.com/actions/toolkit/blob/b36e70495fbee083eb20f600eafa9091d832577d/packages/core/src/file-command.ts#L27-L47
SaschaMann
force-pushed
the
SaschaMann-patch-1
branch
from
b044a90
to
99fb387
Compare
|
Not sure how to fix that last test, please let me know if you have any ideas! |
src/GitHubActions.jl
Outdated
| @@ -188,10 +190,12 @@ Set environment variable `k` to value `v`. | |||
| function set_env(k, v) | |||
| val = cmd_value(v) | |||
| ENV[k] = val | |||
There was a problem hiding this comment.
Setting ENV should probably be moved to be after the safety-precaution checks, imo. I guess it doesn't matter if we're literally going to exit, but if we refactored that to throw or something instead, then we'd have a bug. (And I guess if someone's atexit introspects ENV then we still have a bug currently)
test/runtests.jl
Outdated
| if VERSION.minor < 6 | ||
| mock(atexit) do ae | ||
| set_env("b", "foo$(delimiter)bar") | ||
| @test called_once_with(ae, GHA.fail) |
There was a problem hiding this comment.
I haven't used SimpleMock before, but it looks like called_once_with doesn't mean "called once with args X, and possibly other times", but rather "called once AND that one time it had args X": https://github.com/JuliaTesting/SimpleMock.jl/blob/4ee3c0dea7238349be2a1ce157684118cb55e135/src/mock_type.jl#L136-L142
Therefore I bet the CI failure is that atexit is being called other times too, with other args.
There was a problem hiding this comment.
I haven't used it before either, I copied the other example from the testsuite. I interpreted that docstring to refer to the specific mock, in this case ae, which has only been called once. But I'm not sure.
@christopher-dG do you know how to fix this?
The previous discussion in #6 finally has a proper answer, this updates it to follow
@actions/core: https://github.com/actions/toolkit/blob/b36e70495fbee083eb20f600eafa9091d832577d/packages/core/src/file-command.ts#L27-L47