Feature: Add an optional param to cli to only run requests that have tests #1314

jeff-edmondson-pci · 2024-01-03T16:38:41Z

Description

In a CI/CD environment there exists a use case of only wanting to run requests that have tests.
Added an optional param --tests-only to the CLI arg list. If this is set it will only add request that have tests.

Collection

Result

Contribution Checklist:

The pull request only addresses one issue or adds one feature.
The pull request does not introduce any breaking changes
I have added screenshots or gifs to help explain the change if applicable.
I have read the contribution guidelines.
Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

ahmad-elafify-pci · 2024-01-03T17:08:15Z

+1

mj-h · 2024-01-08T08:53:53Z

Possibly related: #1039

This PR creates a difference between the way the collections are run in the UI vs the CLI. If this PR is merged, I propose to file a follow-up issue to add a similar option to the UI.

However, there are a few questions that this PR raises:

should a file where the tests are all commented out be included? (To me, the answer would be "no", but this PR would include it)
should a file that has assertions but no tests be included? (To me, the answer is "yes", but this PR would skip such a file)
should a file that has tests in the post-response script be included?
- This is messy. The post-response-script and the test-script are run separately, but they do pretty much the same thing. The UI gets confused when I move tests from one to the other. I think this should be cleaned up first before merging this PR -- there should not be two places where post-response scripts are being defined. @helloanoop : What do you think?

Lastly: This PR promotes a collection-writing-style that I would call "bag-of-requests", where some are used for exploratory-testing, and others are used for automatic testing, and the difference between the two is given implicitly.

I would propose a different solution to this problem: In your collection, sort your requests into appropriate subfolders. Then, we should change the runner so that you can do:

bru run my-collection --folder "subfolder with all the tests"

I think that would lead to cleaner collections, and give you more flexibility overall.

jeff-edmondson-pci · 2024-01-08T13:48:53Z

I would propose a different solution to this problem: In your collection, sort your requests into appropriate subfolders. Then, we should change the runner so that you can do:

@mj-h Yes I do agree that it would be easier to just throw all of the tests into a folder but I don't think that is very realistic for complex collections, such as having a very large API collection which many developers are using & contributing to. It is organized by api routes. The structure would be:

Super Cool Sports API
└───Teams
 │   └─── League 1
 |          └─── test1.bru
 │   └─── League 2
└───Games
 │   └─── Scores

Which matches the url path /teams/league-1/test1 for the resource.

Now if we wanted to have the CI/CD run the requests only with tests but only for a given folder as you are suggesting (correct me if I am wrong) we run into the issue of the API collection not remaining organized.
The two work arounds for this could be

Have duplicated request between original location and the tests folder. This is not ideal for a variety of reasons
Just move the request into the tests folder from it's original location. Again not ideal.

Super Cool Sports API
└───Teams
 │   └─── League 1
 │   └─── League 2
└───Games
 │   └─── Scores
└───Tests
 |   └─── test1.bru

Open to suggestions

rabestro · 2024-01-08T15:49:40Z

Open to suggestions

(1) I have many tests with assertion only. I have a test with post-script and assertion.

The proposed implementation skipped them.

(2) The suggested implementation raises the cyclomatic complexity of the code which is not ideal. The implementation involves adding another if/then case, but it is not a recommended approach. Instead, using strategies would be a better option. For instance, we can use a simple strategy (all scripts) and apply different strategies for other cases as needed.

jeff-edmondson-pci · 2024-01-11T14:37:47Z

Asserts were added, @helloanoop let me know if any changes are required, would love to get this in Bruno to allow my team to fully use it!

rabestro · 2024-01-12T08:25:42Z

@jeff-edmondson-pci you always have the opportunity to build your fork and use your version of Bruno with the features you need.

From my point of view, there is at least a lack of documentation correction that would reflect the appearance of new features.

I also think it’s right to first discuss the need for any additions before creating a pull request.

helloanoop · 2024-01-12T11:16:06Z

Thanks @jeff-edmondson-pci for working on the PR and @mj-h and @rabestro everyone for chiming in with comments

should a file where the tests are all commented out be included? (To me, the answer would be "no", but this PR would include it)

@mj-h I agree. This is edge case though. We can iteratively improve this logic later by using the decomment library to address this

should a file that has assertions but no tests be included? (To me, the answer is "yes", but this PR would skip such a file)

@mj-h I agree. An assertion is just a "declarative" way of writing a test. I see that this has been addressed in the PR.

The post-response-script and the test-script are run separately, but they do pretty much the same thing. The UI gets confused when I move tests from one to the other. I think this should be cleaned up first before merging this PR

The separation is intentional. I agree that both these blocks (post response scripts and test scripts) do the same thing. The intent is to visually separate code related to "scripting" and "tests".

scripting - things like parsing a response and updating some collection variables
tests - assertions and tests

I have many tests with assertion only. I have a test with post-script and assertion. The proposed implementation skipped them.

@rabestro This has been addressed in the PR

The suggested implementation raises the cyclomatic complexity of the code which is not ideal. The implementation involves adding another if/then case, but it is not a recommended approach. Instead, using strategies would be a better option. For instance, we can use a simple strategy (all scripts) and apply different strategies for other cases as needed.

@rabestro There is a plan to include tags in the roadmap. I think the business use case of run tests that have tests/assertions is a valid one. Implementing the tags feature would take some time.

Also the --test-only arg being introduced in this PR is optional. My personal take would to be use the concept of tags (which is a pretty common patten across testing frameworks) when that feature lands in Bruno.

@dcoomber @Its-treason @martinsefcik @jeff-edmondson-pci @mj-h @rabestro

The PR is GTG from my side. Let me know if you have further comments. I am planning to have this merged over the weekend.

martinsefcik · 2024-01-15T21:07:20Z

packages/bruno-cli/src/commands/run.js

@@ -215,12 +230,14 @@ const builder = async (yargs) => {
    .example(
      '$0 run request.bru --output results.xml --format junit',
      'Run a request and write the results to results.xml in junit format in the current directory'
-    );
+    )
+    .example('$0 run request.bru --test-only', 'Run all requests that have a test');


On line 214 option name is 'tests-only here in example 'test-only

martinsefcik · 2024-01-15T21:09:01Z

packages/bruno-cli/src/commands/run.js

@@ -200,6 +211,10 @@ const builder = async (yargs) => {
      type: 'boolean',
      description: 'Allow insecure server connections'
    })
+    .option('tests-only', {
+      type: 'boolean',
+      description: 'Only run requests that have a test'


If it runs also in case there are assertions only I would extend this description to be clear when request is executed.

martinsefcik · 2024-01-15T21:19:23Z

@helloanoop Personally I would rather see in Bruno more "standardized" support for tags instead of this feature. If there would be such support then I guess this --tests-only option is not needed.
Also If I put only something like console.log('') inside tests script then it automatically executes such request, but does it mean that such request has tests ? So for me it is a bit confusing, option description is short and not enough and I mostly agree with @rabestro

helloanoop · 2024-01-29T18:15:15Z

Thanks everyone for the feedback and comments.
I have decided to merge this.

I agree that --tests-only (although optional) has some edge cases

Like a request can have console.log('') inside tests script then it automatically executes such request, but does it mean that such request has tests ?
What if a test block is commented out. Does it mean that such request still has tests ?

Its upto the user to ensure that

they have tests in the request and
they are not commented out and
they don't have code in the tests script that are not tests

Add an optional param to cli to only run requests that have tests

cb16b44

jeff-edmondson-pci mentioned this pull request Jan 3, 2024

CLI: add a flag to only run queries with asserts & tests. #1287

Closed

Added to recrusive run

159b41f

jeff-edmondson-pci changed the title ~~Add an optional param to cli to only run requests that have tests~~ Feature: Add an optional param to cli to only run requests that have tests Jan 3, 2024

added asserts to test only flag

67887f6

helloanoop requested review from martinsefcik, Its-treason and dcoomber January 12, 2024 11:17

Its-treason approved these changes Jan 13, 2024

View reviewed changes

martinsefcik reviewed Jan 15, 2024

View reviewed changes

Merge branch 'main' into feature/cli-tests-only

0c32453

helloanoop merged commit 5553875 into usebruno:main Jan 29, 2024
0 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add an optional param to cli to only run requests that have tests #1314

Feature: Add an optional param to cli to only run requests that have tests #1314

jeff-edmondson-pci commented Jan 3, 2024 •

edited

Loading

ahmad-elafify-pci commented Jan 3, 2024

mj-h commented Jan 8, 2024 •

edited

Loading

jeff-edmondson-pci commented Jan 8, 2024 •

edited

Loading

rabestro commented Jan 8, 2024 •

edited

Loading

jeff-edmondson-pci commented Jan 11, 2024

rabestro commented Jan 12, 2024

helloanoop commented Jan 12, 2024 •

edited

Loading

martinsefcik Jan 15, 2024

martinsefcik Jan 15, 2024

martinsefcik commented Jan 15, 2024

helloanoop commented Jan 29, 2024

Feature: Add an optional param to cli to only run requests that have tests #1314

Feature: Add an optional param to cli to only run requests that have tests #1314

Conversation

jeff-edmondson-pci commented Jan 3, 2024 • edited Loading

Description

Collection

Result

Contribution Checklist:

Publishing to New Package Managers

ahmad-elafify-pci commented Jan 3, 2024

mj-h commented Jan 8, 2024 • edited Loading

jeff-edmondson-pci commented Jan 8, 2024 • edited Loading

rabestro commented Jan 8, 2024 • edited Loading

jeff-edmondson-pci commented Jan 11, 2024

rabestro commented Jan 12, 2024

helloanoop commented Jan 12, 2024 • edited Loading

martinsefcik Jan 15, 2024

Choose a reason for hiding this comment

martinsefcik Jan 15, 2024

Choose a reason for hiding this comment

martinsefcik commented Jan 15, 2024

helloanoop commented Jan 29, 2024

jeff-edmondson-pci commented Jan 3, 2024 •

edited

Loading

mj-h commented Jan 8, 2024 •

edited

Loading

jeff-edmondson-pci commented Jan 8, 2024 •

edited

Loading

rabestro commented Jan 8, 2024 •

edited

Loading

helloanoop commented Jan 12, 2024 •

edited

Loading