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 Code formatting check post Pull-Request (#3349) #4028

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

prakhar479
Copy link

  • Bug: added code formatting check in java test pull request hook
    Signed-off-by: Prakhar Singhal [email protected]

Fixes #

* bug: added code formatting check in java test pull request hook
Signed-off-by: Prakhar Singhal <[email protected]>
Copy link

linux-foundation-easycla bot commented Aug 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

knative-prow bot commented Aug 5, 2024

Welcome @prakhar479! It looks like this is your first PR to knative-extensions/eventing-kafka-broker 🎉

@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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2024
Copy link

knative-prow bot commented Aug 5, 2024

Hi @prakhar479. Thanks for your PR.

I'm waiting for a knative-extensions 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-sigs/prow repository.

Copy link

knative-prow bot commented Aug 5, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: prakhar479
Once this PR has been reviewed and has the lgtm label, please assign dsimansk for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@pierDipi
Copy link
Member

pierDipi commented Aug 6, 2024

/cc @Leo6Leo @Cali0707

@knative-prow knative-prow bot requested a review from Leo6Leo August 6, 2024 09:04
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.48%. Comparing base (e9ac303) to head (4909b5d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4028   +/-   ##
=======================================
  Coverage   48.48%   48.48%           
=======================================
  Files         244      244           
  Lines       14469    14469           
=======================================
  Hits         7016     7016           
  Misses       6746     6746           
  Partials      707      707           

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

@prakhar479
Copy link
Author

seems like the java tests are failing. I found following as first point of failure:

Running dev.knative.eventing.kafka.broker.core.file.FileWatcherTest

with error:

Error:  Tests run: 2, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 10.68 s <<< FAILURE! -- in dev.knative.eventing.kafka.broker.core.file.FileWatcherTest

these were some relevant logs between initialization and error:

09:06:16.244 [contract-file-watcher] INFO  dev.knative.eventing.kafka.broker.core.file.FileWatcher - Started watching /tmp/test5718446383111542373.txt
09:06:16.267 [contract-file-watcher] DEBUG dev.knative.eventing.kafka.broker.core.file.FileWatcher - Contract updates
09:06:16.284 [contract-file-watcher] DEBUG dev.knative.eventing.kafka.broker.core.file.FileWatcher - Contract updates
09:06:26.325 [contract-file-watcher] INFO  dev.knative.eventing.kafka.broker.core.file.FileWatcher - Started watching /tmp/test654706480592262681.txt

Can someone help with following since I did not make any changes to core logic and thus am unsure why are these failing. thanks!

@Leo6Leo
Copy link
Contributor

Leo6Leo commented Aug 6, 2024

emm, this flaky test appears again. That's a separate issue, and this is not related to your PR @prakhar479 .

I will look into this and get back to you, thanks for providing the clear description of the error logs!

@pierDipi
Copy link
Member

pierDipi commented Aug 7, 2024

Fix for the flaky test is here #4029

@pierDipi
Copy link
Member

pierDipi commented Aug 7, 2024

/ok-to-test

@knative-prow knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 7, 2024
@prakhar479
Copy link
Author

/retest

@prakhar479
Copy link
Author

/retest-required

@prakhar479
Copy link
Author

prakhar479 commented Aug 8, 2024

@pierDipi @Leo6Leo I have re-ran all tests. let me know if anything else is required. Thanks!

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 starting on this @prakhar479 !

Could you also add this to

function build_tests() {
header "Running control plane build tests"
default_build_test_runner || fail_test "Control plane build tests failed"
}
so that it gets verifies by the blocking test jobs as well as the non-blocking Java unit test you've added it to so far?

@prakhar479
Copy link
Author

prakhar479 commented Aug 24, 2024

Thanks for starting on this @prakhar479 !

Could you also add this to

function build_tests() {
header "Running control plane build tests"
default_build_test_runner || fail_test "Control plane build tests failed"
}

so that it gets verifies by the blocking test jobs as well as the non-blocking Java unit test you've added it to so far?

I have added the formatting check in build_tests as well.

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 24, 2024
Copy link

knative-prow bot commented Aug 25, 2024

@prakhar479: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
upgrade-tests_eventing-kafka-broker_main 62ba7b2 link true /test upgrade-tests
channel-reconciler-tests-sasl-ssl_eventing-kafka-broker_main 62ba7b2 link true /test channel-reconciler-tests-sasl-ssl

Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@prakhar479
Copy link
Author

prakhar479 commented Aug 26, 2024

I am not quite able to understand why the reconciler-tests are failing. Following error in TestKafkaChannelOIDC/KafkaChannel_handles_requests_with_OIDC_tokens_correctly/KafkaChannel_reject_event_for_wrong_OIDC_audience/Assert/KafkaChannel_get_401_on_response denotes a internal error:

- Match errors -
        0 - event status code don't match. Expected: '401', Actual: '0'
        1 - event status code don't match. Expected: '401', Actual: '0'
        2 - event status code don't match. Expected: '401', Actual: '0'
        3 - event status code don't match. Expected: '401', Actual: '500'

and both failed tests return with code 255. I am not quite sure what causes the error. What am I missing @pierDipi @Leo6Leo @Cali0707 ?

Copy link
Contributor

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spotless is not verified on PRs
4 participants