-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tool test: improving coverage, cleanups, minor fixes #474
Conversation
@@ -288,7 +288,7 @@ func (f *InputFile) apply(ac *repb.ActionResult, s *Server, execRoot string) err | |||
if err := os.MkdirAll(filepath.Join(execRoot, filepath.Dir(f.Path)), os.ModePerm); err != nil { | |||
return fmt.Errorf("failed to create input dir %v: %v", filepath.Dir(f.Path), err) | |||
} | |||
err := os.WriteFile(filepath.Join(execRoot, f.Path), nil, 0755) | |||
err := os.WriteFile(filepath.Join(execRoot, f.Path), bytes, 0755) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's from #473
@@ -83,8 +86,8 @@ Platform | |||
|
|||
Inputs | |||
====== | |||
[Root directory digest: e23e10be0d14b5b2b1b7af32de78dea554a74df5bb22b31ae6c49583c1a8aa0e/75] | |||
a/b/input.txt: [File digest: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855/0] | |||
[Root directory digest: 1b1d5b9e3eb97866805deca901299fcdde98b5747e147f35187e12c16e120186/75] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's from #473
@@ -109,55 +112,98 @@ func TestTool_CheckDeterminism(t *testing.T) { | |||
e, cleanup := fakes.NewTestEnv(t) | |||
defer cleanup() | |||
cmd := &command.Command{ | |||
Args: []string{"foo bar baz"}, | |||
Args: []string{"foo", "bar", "baz"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is here (and below) in order to not provide bad examples in our unit tests. E.g. "echo hello world" would not work as a single string; it needs to be split up.
|
||
client := &Client{GrpcClient: e.Client.GrpcClient} | ||
if err := client.CheckDeterminism(context.Background(), acDg.String(), "", 2); err != nil { | ||
t.Errorf("CheckDeterminism returned an error: %v", err) | ||
} | ||
// Now execute again with changed inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was a result of copy-paste; inputs do not actually change, only output does.
Preview of the Node Properties support change: ola-rozenfeld#1 |
Includes #473 because I couldn't figure out how to send a PR chain, sorry!
fakes.InputFile
whenever appropriateExecuteAction*
tests to not depend on implementation of the fake, and instead to mimics the user journey (downloading the inputs locally, then changing them).What I was actually doing is fixing #460, but that PR got a bit too big with unrelated fixes and cleanups, so I decided to split these out.
Thank you!