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

Allow *Testing.T argument before context #3800

Merged

Conversation

maximpertsov
Copy link
Contributor

@maximpertsov maximpertsov commented Apr 11, 2024

Configure revive linter to allow *Testing.T and friends to come before context.Context as a function argument.

Also add and fixup code to adhere to the following default revive rules:

  • superfluous-else
  • unreachable-code

Context: #3795 (comment)

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Apr 11, 2024
@maximpertsov maximpertsov changed the title Allow *.Testing argument before context Allow Testing.* argument before context Apr 11, 2024
@maximpertsov maximpertsov changed the title Allow Testing.* argument before context Allow *Testing.* argument before context Apr 11, 2024
@maximpertsov maximpertsov changed the title Allow *Testing.* argument before context Allow *Testing.T argument before context Apr 11, 2024
@maximpertsov maximpertsov marked this pull request as ready for review April 11, 2024 22:23
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Apr 11, 2024
@maximpertsov maximpertsov force-pushed the lint-revive-test-before-context branch from d61fe1f to 1da25da Compare April 11, 2024 22:34
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Apr 11, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Apr 11, 2024
@@ -131,8 +131,7 @@ func TestState(t *testing.T) {
return nil, errors.New("planning failed")
},
executeFunc: func(ctx context.Context, plan motionplan.Plan) (state.ExecuteResponse, error) {
t.Log("should not be called as planning failed")
t.FailNow()
t.Fatal("should not be called as planning failed") //nolint:revive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping as is with a nolint since this effects benchmarks

Copy link
Contributor

Availability

Scene # viamrobotics:main maximpertsov:lint-revive-test-before-context Percent Improvement Health
1 100% 100% 0%
2 100% 100% 0%
3 100% 100% 0%
4 100% 100% 0%
5 70% 70% 0%
6 50% 50% 0%
7 20% 30% 50%
8 100% 100% 0%
9 100% 90% -10%
10 100% 100% 0%
11 100% 100% 0%
12 100% 100% 0%
13 100% 100% 0%
14 100% 100% 0%
15 100% 100% 0%
16 100% 100% 0%
17 100% 100% 0%
18 80% 70% -12%

Quality

Scene # viamrobotics:main maximpertsov:lint-revive-test-before-context Percent Improvement Probability of Improvement Health
1 1.31±0.00 1.31±0.00 -0% 50%
2 0.91±0.00 0.91±0.00 -0% 50%
3 2.45±0.03 2.44±0.01 0% 63%
4 4.59±0.82 4.59±0.82 -0% 50%
5 13.83±9.65 14.90±9.10 -8% 47%
6 9.25±2.57 11.27±1.72 -22% 26%
7 2.08±0.41 7.66±0.34 -268% 0%
8 5.58±1.40 5.58±1.40 -0% 50%
9 4.99±1.40 4.42±0.20 11% 65%
10 4.20±0.36 4.20±0.36 0% 50%
11 3.13±0.00 3.13±0.00 -0% 50%
12 4.00±0.79 3.91±0.85 2% 53%
13 869.47±143.73 869.73±143.97 -0% 50%
14 1685.05±410.69 1685.05±410.69 -0% 50%
15 57034.21±1352.64 57034.21±1352.64 -0% 50%
16 60745.13±6621.84 60745.13±6621.84 -0% 50%
17 13085.85±2789.97 13085.85±2789.97 0% 50%
18 120845.07±10347.92 120484.47±10545.51 0% 51%

Performance

Scene # viamrobotics:main maximpertsov:lint-revive-test-before-context Percent Improvement Probability of Improvement Health
1 5.50±2.87 5.50±2.87 -0% 50%
2 5.50±2.87 5.50±2.87 -0% 50%
3 5.50±2.87 5.50±2.87 -0% 50%
4 5.50±2.87 5.50±2.87 -0% 50%
5 6.14±2.23 6.14±2.23 -0% 50%
6 5.80±2.48 5.80±2.48 -0% 50%
7 4.00±3.00 3.00±1.63 25% 62%
8 5.50±2.87 5.50±2.87 -0% 50%
9 5.50±2.87 5.89±2.77 -7% 46%
10 5.50±2.87 5.50±2.87 -0% 50%
11 5.50±2.87 5.50±2.87 -0% 50%
12 5.50±2.87 5.50±2.87 -0% 50%
13 5.50±2.87 5.50±2.87 -0% 50%
14 5.50±2.87 5.50±2.87 -0% 50%
15 5.50±2.87 5.50±2.87 -0% 50%
16 5.50±2.87 5.50±2.87 -0% 50%
17 5.50±2.87 5.50±2.87 -0% 50%
18 5.00±2.92 4.57±2.87 9% 54%

The above data was generated by running scenes defined in the motion-testing repository
The SHA1 for viamrobotics:main is: d61fa70b471953d46c530dbc2b507d0d981f0a67
The SHA1 for maximpertsov:lint-revive-test-before-context is: d61fa70b471953d46c530dbc2b507d0d981f0a67

  • 10 samples were taken for each scene
  • A timeout of 5.0 seconds was imposed for each trial

@maximpertsov maximpertsov merged commit e48b684 into viamrobotics:main Apr 12, 2024
17 checks passed
@maximpertsov maximpertsov deleted the lint-revive-test-before-context branch April 12, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants