RSDK-9087 Test setup phase #4483
Conversation
viambot
added
safe to test
maximpertsov
force-pushed
the
RSDK-9087-test-setup-phase
branch
from
4f61647
to
c69e84c
Compare
viambot
added
safe to test
maximpertsov
force-pushed
the
RSDK-9087-test-setup-phase
branch
from
9241b8d
to
1a456f3
Compare
viambot
added
safe to test
maximpertsov
force-pushed
the
RSDK-9087-test-setup-phase
branch
from
d9d985c
to
26e8e24
Compare
viambot
added
safe to test
maximpertsov
force-pushed
the
RSDK-9087-test-setup-phase
branch
from
26e8e24
to
ab997cf
Compare
viambot
added
safe to test
viambot
added
the
safe to test
| @@ -248,6 +249,7 @@ func (m Module) EvaluateFirstRunPath(packagesDir string) ( | |||
|
|
|||
| firstRunSuccessPath := unpackedModDir + FirstRunSuccessSuffix | |||
| if _, err := os.Stat(firstRunSuccessPath); !errors.Is(err, os.ErrNotExist) { | |||
| logger.Info("first run already ran") | |||
There was a problem hiding this comment.
it shouldn't - where do you see potential for a double log?
There was a problem hiding this comment.
because it returns an error saying the same thing - just wondering, wasn't sure if that would happen
There was a problem hiding this comment.
oh yeah, sorry you are right - the error from this function is logged at the debug level, so we get a double log in that scenario. this should get cleared up when we refactor this function. the current contract requires we return an error here to prevent first run logic from firing. we can change the text in the error to prevent a double-log in the meantime.
There was a problem hiding this comment.
since it only happens during debug, I think it's better to address during the upcoming refactor PR
module/testmodule/first_run.sh
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| if [[ -n "$VIAM_TEST_FAIL_RUN_FIRST" ]]; then | ||
| >&2 echo "erroring... 1" |
There was a problem hiding this comment.
can you add a comment to that effect?
This reverts commit b59505e.
|
sorry, which comment/effect are you referring to here? |
viambot
added
safe to test
two other changes: * clarify docs near logs.TakeAll() * handle unsetting environment variables in between sections
viambot
added
safe to test
viambot
added
safe to test
made a function to clarify what's going on: 9f4a614 |
viambot
added
safe to test
done ee08599. unfortunately we still need to sleep in the script for tests to pass - i was hoping this would remove the need :( |
testutils/file_utils.go
Outdated
| // resulting executable binary into a temporary directory. If successful, this function will | ||
| // return the path to the executable binary. | ||
| func BuildTempModule(tb testing.TB, modDir string) string { | ||
| // tb.Helper() |
testutils/file_utils.go
Outdated
| // and "first_run.sh" into the same temporary directory. It is assumed that these files are in the | ||
| // provided module directory. If successful, this function will return the path to the executable binary. | ||
| func BuildTempModuleWithFirstRun(tb testing.TB, modDir string) string { | ||
| // tb.Helper() |
|
ugh, yes great catch. I was briefly debating whether it's worth removing this permanently so we can see exactly which lines of this helper fail. However, that goes against the idea of a helper function and will probably make tests that use it harder to debug in the future. |
viambot
added
safe to test
Summary
Add basic tests for for first run, mainly asserting based on expected logs. Test the following scenarios, in order:
In addition, this PR refactors the
BuildTempModulehelper for clarity.Prerequisites
TODO
io.CopyFix flakes - likely due to inconsistent stdio/stderr echos from first run scriptFixed sources of flakes - for details see RSDK-9087 Test setup phase #4483 (comment)