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

Add command to run single rekt test in the e2e-debug script #7426

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

prakrit55
Copy link
Contributor

Fixes #7204

Proposed Changes

  • Added command to run single test

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note


Docs

Signed-off-by: Griffin <prakritimandal611@gmail.com>
@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 5, 2023
Copy link

knative-prow bot commented Nov 5, 2023

Hi @prakrit55. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/test-and-release Test infrastructure, tests or release labels Nov 5, 2023
Copy link
Member

@Cali0707 Cali0707 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 great start @prakrit55 !

/ok-to-test


go_test_e2e -timeout=30m -run="${test_name}" "${test_dir}/..." || fail_test "Test(s) failed"

success
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
success
success

hack/e2e-debug.sh Outdated Show resolved Hide resolved
hack/e2e-debug.sh Show resolved Hide resolved
@knative-prow knative-prow bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 13, 2023
@knative-prow knative-prow bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2023

header "Running tests"

go_test_e2e -timeout=30m -run="${test_name}" "${test_dir}/..." || fail_test "Test(s) failed"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
go_test_e2e -timeout=30m -run="${test_name}" "${test_dir}/..." || fail_test "Test(s) failed"
go_test_e2e -timeout=30m -run="${test_name}" "${test_dir}" || fail_test "Test(s) failed"

Copy link
Member

Choose a reason for hiding this comment

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

@Leo6Leo I don't agree with this change, the /... allows for matching tests in subdirectories to also be run. I think leaving this as is might make it easier for users to use the e2e-debug.sh script. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

The reason why I remove that is:

  1. We already specify which test to run by passing test_name.
  2. If we add the /..., which means a lot of "empty" tests will be run unless we specify the exact folder that the test_name is located

Example:
With the command ./hack/e2e-debug.sh TestBrokerWithManyTriggers ./test/rekt

Running go test with args: -tags=e2e -count=1 -race -timeout=30m -run=TestBrokerWithManyTriggers ./test/rekt/...

EMPTY test/rekt/features/sequence
EMPTY test/rekt/features/sinkbinding
EMPTY test/rekt/features/source
EMPTY test/rekt/features/trigger
EMPTY test/rekt/features/knconf
EMPTY test/rekt/features/broker
EMPTY test/rekt/resources/addressable
EMPTY test/rekt/features/channel
EMPTY test/rekt/resources/account_role
PASS test/rekt.TestBrokerWithManyTriggers/Broker_with_many_triggers/test_default_broker_with_many_attribute_triggers/Prerequisite (0.00s)
...

To address your point:
We could introduce a flag or an environment variable that decides whether to include subdirectories in the test run. This way, users can choose the behavior that best suits their current needs, whether it's running a specific test or a broader suite of tests. This will balances the need for specificity and efficiency with the flexibility to run more comprehensive tests when needed. how does that sound? @Cali0707

Copy link
Member

Choose a reason for hiding this comment

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

I guess I personally think the EMPTY tests aren't an issue, but if you prefer it this way then I am happy to go with your preference

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c8f4624) 76.74% compared to head (0ad46df) 76.79%.
Report is 26 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7426      +/-   ##
==========================================
+ Coverage   76.74%   76.79%   +0.04%     
==========================================
  Files         253      253              
  Lines       13916    14098     +182     
==========================================
+ Hits        10680    10826     +146     
- Misses       2702     2732      +30     
- Partials      534      540       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Griffin <prakritimandal611@gmail.com>
@Leo6Leo
Copy link
Member

Leo6Leo commented Nov 17, 2023

Great work and thanks for the contribution @prakrit55 !
I don't see any problems with this code
/lgtm
/cc @pierDipi @Cali0707 @creydr

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2023
@Cali0707 Cali0707 changed the title Add command to run single rekt test in the e2e-common script Add command to run single rekt test in the e2e-debug script Nov 22, 2023
Copy link

knative-prow bot commented Nov 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, prakrit55

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2023
@knative-prow knative-prow bot merged commit 46933d0 into knative:main Nov 23, 2023
35 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add command to run single rekt test in e2e-debug script
4 participants