Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Remove user supplied -run flags when running tests #2285

Merged
merged 13 commits into from
Oct 7, 2019

Conversation

scriptonist
Copy link
Contributor

Towards: #2260

I've tried to address the bug in accordance with the issue comments. Given that I'm very new to the codebase and TS in general, there should be ample scope for improvement. I would really appreciate feedback and suggestions.

@msftclas
Copy link

msftclas commented Jan 29, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @scriptonist!

Looks like this is your first contribution to this project, Thanks & Welcome!

This project makes an attempt to separate out test related utility functions from others by having them in the testUtils.ts file. So that would be my first feedback.

On second thought, I believe we can fix this bug by updating the existing getTestFlags function instead of introducing a new function. What do you think of adding an optional parameter (default to false) to getTestFlags that will control whether or not the run parameter should be removed from the user provided flags?

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For below features, we dont want to use the user provided -run flag

  • goBuild.ts: To run go test to file build errors in test file
  • goRunTestCodelens.ts: To create the codelens links above test functions
  • goTest.ts Run test at cursor or run all tests in file

I believe in all other cases, we should respect the user provided -run flag.

Thoughts?

@ramya-rao-a
Copy link
Contributor

Hey @scriptonist

We haven't heard back from you in a while, so I have taken the liberty to tweak your PR a bit and take it to conclusion.

I have reverted the commits that were made due to my earlier suggestion to make getTestFlags take an arg to decide whether -run should be removed or not.

I realized that this forces us to think from each entry point and instead there is a simpler way of doing this by removing the user provided -run flag only when the extension explicitly uses it.

Thanks for the work you put in here earlier

Happy Coding!

@ramya-rao-a ramya-rao-a merged commit 83b8fea into microsoft:master Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants