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

Testcript enhancements #105

Closed
wants to merge 2 commits into from
Closed

Testcript enhancements #105

wants to merge 2 commits into from

Conversation

verdverm
Copy link

@verdverm verdverm commented Jun 6, 2020

This adds some requests in the issues / PRs plus a number of other things

what do you think?


Script upgrades:

  • ~ for line comments (need to write messages for devs in test files)
  • ? for ignoring exit codes, changes neg from bool to int
  • status for checking exit / status codes
  • http call with many args supported, also client management to support named default setups and multiple client connections
  • call allows users to add functions that act like commands without exec (useful for coverage)

Config options:

  • Glob param option
  • Funcs param option for call
  • PhasePrefix for configuring phase indicator
  • CommentPrefix for configuring the comment prefix

@mvdan
Copy link
Collaborator

mvdan commented Jun 6, 2020

I wonder - why did you not raise an issue about these features first? We generally discuss new APIs and features before going straight into code changes.

For example, I don't think http really belongs in there. This is a generic test framework for Go commands in general, so http isn't really a necessity - and you can always add extra commands to do any custom logic you want.

@mvdan
Copy link
Collaborator

mvdan commented Jun 6, 2020

I also hadn't quite realised how large this pull request is. With such a large refactor and so much new code, I really do wish you had discussed this in the issue tracker first.

For example, your ? change has already had some discussion in #93, so it's unfortunate to ignore that discussion and tackle the problem along with a dozen other non-trivial changes all at once.

@verdverm
Copy link
Author

verdverm commented Jun 6, 2020

@mvdan largely this is for visibility as I had not expected this repository to be that active. Many of the discussions have not seen movement in a long time (I did read them).

I am personally using this package for testing non-go applications where this is extremely useful. I am able to simple drop a go.mod and a few _test.go files around and have this awesome testing capability. Given that many people had requested these features:

  1. I forked the project
  2. made the changes
  3. published a replace-able version
  4. made this PR incase others find it useful

It should be 100% backwards compatible

@mvdan
Copy link
Collaborator

mvdan commented Jun 6, 2020

I'm not sure if I follow. For example, that ? issue had comments posted just three weeks ago.

I appreciate that you put a lot of effort into this, I'm just noting that contributing with upstreams works best when the changes are coordinated and posted in small chunks.

@verdverm
Copy link
Author

verdverm commented Jun 6, 2020

Well, it only took a couple of hours, so it's not really that much work, so three weeks does seem like a long time, give how easy this was.

If changes of this nature are not desired, I can maintain a fork or more likely pull it into my growing monotool. Cuelang is something I really want to do in these scripts as well.

@myitcv
Copy link
Collaborator

myitcv commented Jun 7, 2020

Well, it only took a couple of hours, so it's not really that much work, so three weeks does seem like a long time, give how easy this was.

The reason for the apparent "lack of progress" or activity on that issue (or indeed many others) is not really a function of people not having the time to write the code. Instead it's a function of spending time thinking about the solution.

If changes of this nature are not desired, I can maintain a fork or more likely pull it into my growing monotool. Cuelang is something I really want to do in these scripts as well.

As @mvdan has pointed out, there are two mechanisms by which the testscrtipt "API" can be extended with custom commands, without requiring changes to the testscript package itself. If commands are common to many projects, such that it makes sense adding them to the testscript package, then we can cross that bridge when we come to it. But I don't think we're at that point - to my recollection, this is the first time someone has suggested adding http for example.

@verdverm verdverm closed this Jun 7, 2020
@verdverm
Copy link
Author

verdverm commented Jun 7, 2020

forget that I had PR'd anything...

@verdverm
Copy link
Author

verdverm commented Jun 7, 2020

If you have any interest in following along hofstadter-io/hof#56

@verdverm
Copy link
Author

@mvdan @myitcv if this PR had been a "discussion" like we have on the Cue repo, would that be a better way to present something like this?

i.e. Do you also think the new "discussions" feature would benefit from having creation options like:

  • issue like, with nested comments (the current and only option)
  • PR like, so we can talk about code
  • non-QA oriented, all discussions have a Q/A component, but not all discussions are in actuality a Q/A

There is a feedback link at the bottom of the Cue discussions page where I have submitted the above

@myitcv
Copy link
Collaborator

myitcv commented Jun 16, 2020

As @mvdan noted, an issue in the issue tracker would suffice as a starting point. If it makes sense to discuss code a PR could follow.

Discussions is not GA yet, in any case I think an issue would be more appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants