Skip to content
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

cmd/go: it would be great if testScript.cmdExists could check for empty files #54318

Closed
bep opened this issue Aug 6, 2022 · 9 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bep
Copy link
Contributor

bep commented Aug 6, 2022

See

func (ts *testScript) cmdExists(want simpleStatus, args []string) {

I'm using this via https://github.com/rogpeppe/go-internal/tree/master/testscript

And I do understand that one reason why you're having this in an internal package is that you don't want bug reports about it, but:

  • The testscript package is incredibly useful.
  • I had some tests passing that shouldn't – where the files were empty.
  • Maybe this could discover some issues in this repo, too (I tried to add this check and run the tests, but I ... failed to run the tests on my MacBook)
  • Maybe add a flag e.g. -zerosize for the test cases where the file should be empty.

/cc @rogpeppe

@ianlancetaylor
Copy link
Contributor

Perhaps instead of exists filename you could write grep '.' filename.

@bep
Copy link
Contributor Author

bep commented Aug 6, 2022

Perhaps instead of exists filename you could write grep '.' filename.

Yea, maybe, or I could write my own exists command (which currently do not allow me to use the negate keyword), but the thing is:

  • exists already have some extra check flags in there (-readonly, -exec).
  • an empty file would, in my head, in most situations be a red flag I would want a test to shout about.

@thanm
Copy link
Contributor

thanm commented Aug 8, 2022

Would you like to send a patch for this?

@thanm thanm added this to the Backlog milestone Aug 8, 2022
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 8, 2022
@seankhliao
Copy link
Member

cc @bcmills @mvdan

@bep
Copy link
Contributor Author

bep commented Aug 9, 2022

@thanm I may when I find time, but my first attempt running the tests inside cmd/go was a big failure, so I need to look into that first.

@mvdan
Copy link
Member

mvdan commented Aug 9, 2022

I agree with @ianlancetaylor: you can use grep for this. ! grep . stdout is a common way to test that a command didn't print to stdout, for example. And the exists command isn't called notempty, after all.

@bep
Copy link
Contributor Author

bep commented Aug 9, 2022

That would mean doing this for every file:

# Check that it exists an is executable
check -exec filename
# Check that it's not empty (which I assume means reading the entire file)
grep . filename
# ... next file

And the exists command isn't called notempty, after all.

Well, it's not called readonly, either.

This is not a big deal for me, but I would suggest a lower barrier for proposals for non-breaking changes in internal test APIs. If, by adding these 2 lines, you discover a fatal bug in File.Sync() on some platform, it would be well worth it.

@mvdan
Copy link
Member

mvdan commented Aug 9, 2022

What I mean is that the default shouldn't change to check for non-empty files. The "readonly" check is opt-in via a flag, for example. Maybe -notempty?

@bcmills
Copy link
Member

bcmills commented Aug 23, 2022

I've been taking some steps to properly extract the cmd/go script engine (https://go.dev/cl/419875) in order to reuse it for #27494. The approach I've been taking allows the registration of arbitrary commands, and also allows the internal commands to be extended by replacing their implementation.

With that approach, you could modify your own script's exists command to support whatever extra options you want. 🙂

At any rate, I don't think we should make this change in cmd/go itself until / unless we need to use it in a cmd/go test. (The script engine is intentionally internal to cmd/go for the time being. Perhaps at some point we can file a proposal to move it to x/tools or x/exp or someplace like that.)

@bcmills bcmills closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2022
@golang golang locked and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants