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

time freezing doesn't work properly with zyte-common-items #125

Open
kmike opened this issue Jan 31, 2023 · 6 comments
Open

time freezing doesn't work properly with zyte-common-items #125

kmike opened this issue Jan 31, 2023 · 6 comments

Comments

@kmike
Copy link
Member

kmike commented Jan 31, 2023

I tried to use scrapy-poet's savefixture together with Product item from zyte-common-items. The result:

meta.json:

{
    "frozen_time": "2023-01-31T17:25:54.362413+00:00"
}

output.json:

    "metadata": {
        "dateDownloaded": "2023-01-31T17:25:55Z",
        "probability": 1.0
    },

And then, comparison fails when running pytest, on a freshly generated fixture:

E         -  'metadata': {'dateDownloaded': '2023-01-31T17:25:55Z', 'probability': 1.0},
E         ?                                                    ^
E         +  'metadata': {'dateDownloaded': '2023-01-31T17:25:54Z', 'probability': 1.0},
E         ?                                                    ^

Seems to be some issue with rounding, but I'm not sure; haven't checked it. Another possibile explanation is that without freezing the time during test generation the time can change.

@wRAR
Copy link
Member

wRAR commented Jan 31, 2023

We should remove fractional seconds from frozen_time. But it's also because the time ticks.

If all time values we use and compare have precision only in seconds, time ticking for less that a second will not cause problems. If a test runs (i.e. the page object generates the item) for longer than 1 second, we need a way to compare values with a margin, as discussed earlier.

@kmike
Copy link
Member Author

kmike commented Jan 31, 2023

If all time values we use and compare have precision only in seconds, time ticking for less that a second will not cause problems.

Wouldn't it be an issue for faster tests if the current time is XX:XX:59.9999, and then time moves forward?

I wonder if we need some weird workaround, e.g. waiting for a minute to start before recording the time :)

@kmike
Copy link
Member Author

kmike commented Jan 31, 2023

What's the workaround for the users if the issue happens (e.g. because extraction takes more than a second)? Is it to change either frozen_time or dateDownloaded manually to make them match?

@wRAR
Copy link
Member

wRAR commented Jan 31, 2023

Wouldn't it be an issue for faster tests if the current time is XX:XX:59.9999, and then time moves forward?

If frozen_time is XX:XX:59, the "current time" will start at XX:XX:59 (both when generating and when testing), the fractions of a second won't matter. This is because savefixture saves the current time and runs the test generation under time_machine with time set to that value and then exports the same value as frozen_time. We just need to set microseconds of that saved time value to 0.

What's the workaround for the users if the issue happens (e.g. because extraction takes more than a second)? Is it to change either frozen_time or dateDownloaded manually to make them match?

For now? Probably yes, but only if you now that e.g. dateDownloaded is always 1 second later than frozen_time. If it's not that deterministic, I don't see a good workaround.

@kmike
Copy link
Member Author

kmike commented Feb 2, 2023

Partially addressed by scrapinghub/scrapy-poet#122.

@wRAR
Copy link
Member

wRAR commented Feb 22, 2023

Unfortunately having test generation take more than 1 second is easy, especially with additional requests, when using scrapy-poet for that. Initializing the spider and waiting for the server response, or several responses, can easily take more than 1 second.

Though it's easy to change dateDownloaded in output.json to be equal to frozen_time manually after the test generation.

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

No branches or pull requests

2 participants