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

[TEST] Add RowFilteringTestBase to improve the test coverage #4497

Closed
wants to merge 2 commits into from

Conversation

yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Mar 13, 2023

Why are the changes needed?

Add RowFilteringTestBase to improve the test coverage

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@yaooqinn
Copy link
Member Author

cc @bowenliang123

BTW, can you take a look at the mvnd failure?

Error: Exception in thread "main" org.mvndaemon.mvnd.common.DaemonException$StaleAddressException: Could not receive a message from the daemon.
Daemon id: 83189804

@codecov-commenter
Copy link

Codecov Report

Merging #4497 (4d1834b) into master (5aed270) will increase coverage by 0.12%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4497      +/-   ##
============================================
+ Coverage     53.20%   53.32%   +0.12%     
  Complexity       13       13              
============================================
  Files           569      571       +2     
  Lines         31055    31285     +230     
  Branches       4194     4219      +25     
============================================
+ Hits          16522    16682     +160     
- Misses        12973    13024      +51     
- Partials       1560     1579      +19     

see 32 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bowenliang123
Copy link
Contributor

bowenliang123 commented Mar 13, 2023

cc @bowenliang123

BTW, can you take a look at the mvnd failure?

Error: Exception in thread "main" org.mvndaemon.mvnd.common.DaemonException$StaleAddressException: Could not receive a message from the daemon.
Daemon id: 83189804

Let's rerun the failed job this time. It seems mvnd daemon crashes due to memory usage or concurrency. Will consider increasing mvnd.maxHeapSize and lower the concurrency. Relevant discussion: apache/maven-mvnd#161
If the situation continues, we could use mvnd in serial mode or rollback to use mvn instead of mvnd.

@yaooqinn
Copy link
Member Author

I tried serval times but consistently failed

@bowenliang123
Copy link
Contributor

I tried serval times but consistently failed

Yes, I noticed. But curiously the same job for other PR passes.
How about merge this PR without style check this time.

Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@yaooqinn yaooqinn added this to the v1.8.0 milestone Mar 13, 2023
@yaooqinn yaooqinn self-assigned this Mar 13, 2023
@pan3793 pan3793 closed this in 8267108 Mar 14, 2023
@pan3793
Copy link
Member

pan3793 commented Mar 14, 2023

Thanks, merged to master

@yaooqinn yaooqinn deleted the rf branch March 14, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants