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

Fix flaky award queue test #2093

Merged
merged 1 commit into from May 9, 2019

Conversation

2 participants
@Ronolibert
Copy link
Contributor

commented May 7, 2019

Description

Add back the missing best value score to the data setup

Setup

make server_test

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@Ronolibert Ronolibert requested review from chrisgilmerproj and mkrump May 7, 2019

var err error
lastTSPP, err = testdatagen.MakeTSPPerformance(suite.DB(), testdatagen.Assertions{
TransportationServiceProviderPerformance: models.TransportationServiceProviderPerformance{
TransportationServiceProvider: tsp,
TransportationServiceProviderID: tsp.ID,
TrafficDistributionListID: tdl.ID,
BestValueScore: score,
LinehaulRate: rate,
SITRate: rate,

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 7, 2019

Contributor

Based on the change in fff52f0#diff-8ac0c8f373177966abad43f58193ccc0 I'd expect to see more of these fixed up. But maybe the difference is that the other tests use QualityBands like in bf6faa1#diff-8ac0c8f373177966abad43f58193ccc0 ? Can you explain what's going on here?

This comment has been minimized.

Copy link
@Ronolibert

Ronolibert May 7, 2019

Author Contributor

I don't think I could provide a good explanation, but when I was initially doing the refactor most of the properties were not "required" to get the specific test cases to pass. I only then added the QualityBand and the rest of the properties when I came across tests that needed them. Behavior seemed to be exactly the same whether QualityBand was defaulted 0 or explicitly 1 in most cases

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 7, 2019

Contributor

The tests are probably fine and it's probably an artifact of moving from passing a bunch of performance data to passing a struct. But it still makes me nervous when these things change, especially in the award queue of all places.

This comment has been minimized.

Copy link
@Ronolibert

Ronolibert May 7, 2019

Author Contributor

Yeah for sure! I'd like to understand the award queue and what it does more in the future

@@ -603,12 +604,18 @@ func (suite *AwardQueueSuite) Test_AssignTSPsToBands() {
var lastTSPP models.TransportationServiceProviderPerformance
for i := 0; i < tspsToMake; i++ {
tsp := testdatagen.MakeDefaultTSP(suite.DB())
score := float64(i + 1)
rate := unit.NewDiscountRateFromPercent(45.3)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 7, 2019

Contributor

In the old commit score was mps + i + 1. What was mps? And does it matter here?

fff52f0#diff-8ac0c8f373177966abad43f58193ccc0L536

This comment has been minimized.

Copy link
@Ronolibert

Ronolibert May 7, 2019

Author Contributor

I added the mps back and pushed the change! In this case it doesn't matter though since the mps constant is always 0

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 7, 2019

Contributor

Huh. Well thanks! Interesting that its usually 0.

@Ronolibert Ronolibert force-pushed the roc-#165803191-flaky-awardqueue-test branch from d42d480 to 11b67f5 May 7, 2019

@Ronolibert Ronolibert requested a review from chrisgilmerproj May 7, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

Thanks for the quick fix!

@chrisgilmerproj chrisgilmerproj referenced this pull request May 7, 2019

Merged

Add go-vet shadow variable checking. #2042

3 of 3 tasks complete

@Ronolibert Ronolibert force-pushed the roc-#165803191-flaky-awardqueue-test branch from 11b67f5 to 8062632 May 8, 2019

@Ronolibert Ronolibert merged commit f0fccf4 into master May 9, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 53315ea...8062632
Details
codecov/project/go 60.12% remains the same compared to 53315ea
Details

@Ronolibert Ronolibert deleted the roc-#165803191-flaky-awardqueue-test branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.