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

Calculate fake import start from import end #5060

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

phil-davis
Copy link
Contributor

Q A
Bug fix? Y
New feature? N
Related user documentation PR URL N/A
Related developer documentation PR URL N/A
Issues addressed (#s or URLs) #5059
BC breaks?
Deprecations?

Description:

  1. Calculate fakeImportStartDate() as an offset from the end date-time, rather than the current date-time. That should avoid rounding issues due to the real time delay between setting end date-time and then making the adjustment to fake the start date-time.

  2. Fix a bunch of comment typos in Import.php while we are here.

Steps to reproduce the bug:

  1. run the unit tests lots of times
  2. see the failure:
There was 1 failure:
1) Mautic\LeadBundle\Tests\Entity\ImportTest::testGetRunTimeSeconds
Failed asserting that 601 is identical to 600.

Steps to test this PR:

  1. run the unit tests lots more times
  2. do not see any failure

@phil-davis phil-davis changed the title Calculate fake import start from import end [WIP] Calculate fake import start from import end Sep 29, 2017
@phil-davis phil-davis force-pushed the a-testGetRunTimeSeconds branch 2 times, most recently from 524d59b to d217275 Compare September 29, 2017 12:29
@phil-davis phil-davis changed the title [WIP] Calculate fake import start from import end Calculate fake import start from import end Sep 29, 2017
@phil-davis
Copy link
Contributor Author

I suspect that for this test to have failed, it needed to start at something like 12:15:10.995 and have the start time backed up 600 seconds to 12:05:10.995 and then the end time like 12:15:11.005
I think the fractional seconds disappear in the DateTime object, so it ends up with start time 12:05:10 and end time 12:15:11 and the elapsed time is calculated as 601 seconds.

@@ -162,10 +162,10 @@ public function testGetRunTimeSeconds()

$this->assertSame(0, $import->getRunTimeSeconds());

$this->fakeImportStartDate($import, 600);

$import->end(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put a small delay before here in the original code, e.g.:

usleep(100000);

and then run the test and you get about 10% fails.

With just the natural compute time delay it is a bit difficult to get the "random" fail.

@phil-davis phil-davis force-pushed the a-testGetRunTimeSeconds branch 3 times, most recently from 8f29554 to 047e5e4 Compare October 7, 2017 10:11
@phil-davis
Copy link
Contributor Author

Rebased

@phil-davis phil-davis force-pushed the a-testGetRunTimeSeconds branch 2 times, most recently from 2eb0dc2 to 8371db6 Compare November 23, 2017 10:16
@phil-davis
Copy link
Contributor Author

Rebased on top of #5367 so that CI should pass.

@phil-davis
Copy link
Contributor Author

Rebased again

@alanhartless alanhartless added this to the 2.12.1 milestone Dec 13, 2017
@alanhartless alanhartless added pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test labels Dec 19, 2017
@javjim javjim self-assigned this Dec 19, 2017
@Hadamcik
Copy link
Contributor

Works for me, even after 1000 runs of test, nothing brakes (I had usleep(100000); added)

@Hadamcik Hadamcik merged commit 571f20c into mautic:staging Dec 20, 2017
@dbhurley dbhurley removed the ready-to-test PR's that are ready to test label Dec 20, 2017
@Hadamcik Hadamcik removed the pending-test-confirmation PR's that require one test before they can be merged label Dec 20, 2017
@phil-davis phil-davis deleted the a-testGetRunTimeSeconds branch December 20, 2017 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants