Fixed some factory issues to address Percy's false positive flags #11771

mmmavis · 2024-01-25T23:01:42Z

I used this past Percy test as a reference to identify false-positive flags we have been getting recently that are caused by factory configuration

This PR

fixes how to factory generate dates so they are relative to a fixed date (2020-01-01) instead of "now"
fixes how factory generates MozFest's ticket snippet's cost value

Related to #10328

┆Issue is synchronized with this Jira Task

mmmavis · 2024-01-25T23:07:30Z

network-api/networkapi/highlights/factory.py

@@ -18,17 +18,12 @@ class Meta:
            "footer_sentence",
        )

-    class Params:


These details seem a bit unnecessary to me. I'm deleting them to avoid complication.

These are helpful if creating these in a test. Instead of doing something like HighlightFactory(publish_after=timedelta(datetime.now() + 1)) to indicate an unpublished highlight, we can just do HighlightFactory(unpublished=True) and it will create it for us with a publish_after in the future.

I don't think it's worth deleting them, they are helpful factory traits

Cool I'll add them back!

mmmavis · 2024-01-25T23:07:52Z

network-api/networkapi/news/factory.py

@@ -32,34 +32,15 @@ class Meta:
        exclude = ("headline_sentence",)

    class Params:
-        unpublished = Trait(


Similarly, these details seem a bit unnecessary to me. I'm deleting them to avoid complication.

I would like to keep these as well

Will add them back!

mmmavis · 2024-01-25T23:09:36Z

network-api/networkapi/news/factory.py

        video = Trait(is_video=True)

    headline = LazyAttribute(lambda o: o.headline_sentence.rstrip("."))
    outlet = Faker("company")
-    date = Faker("date") if RANDOM_SEED and not TESTING else Faker("past_date", start_date="-30d")
+    date = Faker("past_date", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


I'm unsure why we needed to have two ways to generate values for date here. Any insights?

I'm not sure... it seems to be generating a random date in the past 30 days for testing while for all other generations it uses a random date with no restrictions

mmmavis · 2024-01-25T23:10:15Z

network-api/networkapi/news/factory.py

-        if RANDOM_SEED and not TESTING
-        else Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)
-    )
+    publish_after = Faker("past_datetime", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


Same question here. I'm curious why RANDOM_SEED and TESTING matter when it comes to generating value for publish_after

I'm not sure either. I think we're safe to simplify this if the Python tests pass.

mmmavis · 2024-01-25T23:10:54Z

network-api/networkapi/wagtailpages/factory/blog.py

-        Faker("date_time", tzinfo=timezone.utc)
-        if RANDOM_SEED and not TESTING
-        else Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)
+    first_published_at = Faker("past_datetime", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


Same q as above.

…ate 2020, 1, 1 instead of 'today'

…oundation.mozilla.org into 10328-percy-flags-date

jhonatan-lopes

Hey @mmmavis, thanks for these changes. I'm a bit concerned that these won't be enough to solve the issue though. The dates randomly changing on RCC/Research Hub does not seem to be caused by the factory date generation that you changed here, for instance.

I think it relates more to controlling the randomness. If the randomness is indeed reproducible, the dates shouldn't be changing like that from one Percy test to another.

jhonatan-lopes · 2024-01-26T10:45:18Z

network-api/networkapi/highlights/factory.py

@@ -18,17 +18,12 @@ class Meta:
            "footer_sentence",
        )

-    class Params:


These are helpful if creating these in a test. Instead of doing something like HighlightFactory(publish_after=timedelta(datetime.now() + 1)) to indicate an unpublished highlight, we can just do HighlightFactory(unpublished=True) and it will create it for us with a publish_after in the future.

jhonatan-lopes · 2024-01-26T10:45:49Z

network-api/networkapi/highlights/factory.py

@@ -18,17 +18,12 @@ class Meta:
            "footer_sentence",
        )

-    class Params:


I don't think it's worth deleting them, they are helpful factory traits

jhonatan-lopes · 2024-01-26T10:52:05Z

network-api/networkapi/highlights/factory.py

-    publish_after = Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)
+    publish_after = Faker("past_datetime", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


I'm not sure how this would solve the problem though... 🤔 we would still get a random date but now between Jan 1st 2020 and the current date instead of the last 30 days. We might need a different approach

jhonatan-lopes · 2024-01-26T10:53:02Z

network-api/networkapi/news/factory.py

@@ -32,34 +32,15 @@ class Meta:
        exclude = ("headline_sentence",)

    class Params:
-        unpublished = Trait(


I would like to keep these as well

jhonatan-lopes · 2024-01-26T10:55:03Z

network-api/networkapi/news/factory.py

        video = Trait(is_video=True)

    headline = LazyAttribute(lambda o: o.headline_sentence.rstrip("."))
    outlet = Faker("company")
-    date = Faker("date") if RANDOM_SEED and not TESTING else Faker("past_date", start_date="-30d")
+    date = Faker("past_date", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


I'm not sure... it seems to be generating a random date in the past 30 days for testing while for all other generations it uses a random date with no restrictions

jhonatan-lopes · 2024-01-26T10:56:24Z

network-api/networkapi/news/factory.py

-        if RANDOM_SEED and not TESTING
-        else Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)
-    )
+    publish_after = Faker("past_datetime", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


I'm not sure either. I think we're safe to simplify this if the Python tests pass.

jhonatan-lopes · 2024-01-26T10:56:42Z

network-api/networkapi/wagtailpages/factory/blog.py

-        Faker("date_time", tzinfo=timezone.utc)
-        if RANDOM_SEED and not TESTING
-        else Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)
+    first_published_at = Faker("past_datetime", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)


mmmavis · 2024-01-26T20:46:28Z

Hi @jhonatan-lopes , thanks for the feedback! I'll add the Trait stuff back.

Regarding the date generation issue - from my observation in the past, the date difference seemed always to be "x days" from when the main branch was last updated. For example, if the last Percy build for main was on January 1, 2023 and an article's published date was generated to be June 1, 2020, if I ran a Percy build again on the next day (Jan 2, 2023) then the same article's published date would become June 2, 2020. I suspected the issue was because we used "today" as the baseline to generate the date (e.g., Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)) and that became a variable (with changing value everyday) used in faker's algorithm which ultimately caused the "x days" difference if we ran the Percy build x days apart. I'm thinking by having a fixed date as baseline will always give us an expected value given that the same SEED value is provided, just like how get_random_option works. That said, as a proof of concept I ran another Percy build today on this PR and compared the results with yesterday's build. There are still some leftover issues I need to address. I'm gonna revise my code and have this PR open for another few days to keep my proof of concept going before re-requesting a new review from you. 😁

jhonatan-lopes · 2024-01-26T20:59:18Z

That sounds great, thanks @mmmavis! I tried some stuff locally here but couldn't get to the bottom of the problem either but your comment definitely does make sense. I'll try some other stuff with that in mind (trying to "fudge today's date" and re-generating data) to see if I can repeat the problem

mmmavis force-pushed the 10328-percy-flags-date branch from d079754 to c794da8 Compare January 25, 2024 23:05

mmmavis commented Jan 25, 2024

View reviewed changes

mmmavis force-pushed the 10328-percy-flags-date branch from c794da8 to faf23d0 Compare January 25, 2024 23:32

fixed how to factory generate dates so they are relative to a fixed d…

5cd4015

…ate 2020, 1, 1 instead of 'today'

mmmavis force-pushed the 10328-percy-flags-date branch from 0c04f73 to 5cd4015 Compare January 25, 2024 23:40

fixed how factory generates MozFest's ticket snippet's cost value

6b53875

mmmavis changed the title ~~[WIP] Fixed how to factory generate dates~~ [WIP] Fixed some factory issues to address Percy's false positive flags Jan 25, 2024

mmmavis added 2 commits January 25, 2024 15:47

removed an unused import

dac1db5

Merge branch 'main' into 10328-percy-flags-date

6323908

mmmavis added the run visual regression tests label Jan 25, 2024

mmmavis added 4 commits January 25, 2024 15:54

fixed linitng error

d6fc39e

Merge branch '10328-percy-flags-date' of https://github.com/mozilla/f…

e156704

…oundation.mozilla.org into 10328-percy-flags-date

typo...

54e1212

updated fixed values used in Playwright tests

c5cf7b9

mmmavis added run visual regression tests and removed run visual regression tests labels Jan 26, 2024

mmmavis changed the title ~~[WIP] Fixed some factory issues to address Percy's false positive flags~~ Fixed some factory issues to address Percy's false positive flags Jan 26, 2024

mmmavis marked this pull request as ready for review January 26, 2024 07:16

mmmavis requested a review from jhonatan-lopes January 26, 2024 07:16

jhonatan-lopes reviewed Jan 26, 2024

View reviewed changes

mmmavis added run visual regression tests and removed run visual regression tests labels Jan 26, 2024

Merge branch 'main' into 10328-percy-flags-date

df4517d

mmmavis removed the run visual regression tests label Jan 31, 2024

mmmavis added the run visual regression tests label Jan 31, 2024

Merge branch 'main' into 10328-percy-flags-date

245ac11

mmmavis added run visual regression tests and removed run visual regression tests labels Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed some factory issues to address Percy's false positive flags #11771

Fixed some factory issues to address Percy's false positive flags #11771

mmmavis commented Jan 25, 2024 •

edited by data-sync-user

Loading

mmmavis Jan 25, 2024 •

edited

Loading

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

mmmavis Jan 26, 2024

mmmavis Jan 25, 2024

jhonatan-lopes Jan 26, 2024

mmmavis Jan 26, 2024

mmmavis Jan 25, 2024

jhonatan-lopes Jan 26, 2024

mmmavis Jan 25, 2024

jhonatan-lopes Jan 26, 2024

mmmavis Jan 25, 2024

jhonatan-lopes Jan 26, 2024

jhonatan-lopes left a comment

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

jhonatan-lopes Jan 26, 2024

mmmavis commented Jan 26, 2024 •

edited

Loading

jhonatan-lopes commented Jan 26, 2024

		publish_after = Faker("past_datetime", start_date="-30d", tzinfo=timezone.utc)
		publish_after = Faker("past_datetime", start_date=datetime(2020, 1, 1), tzinfo=timezone.utc)

Fixed some factory issues to address Percy's false positive flags #11771

Are you sure you want to change the base?

Fixed some factory issues to address Percy's false positive flags #11771

Conversation

mmmavis commented Jan 25, 2024 • edited by data-sync-user Loading

mmmavis Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhonatan-lopes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmmavis commented Jan 26, 2024 • edited Loading

jhonatan-lopes commented Jan 26, 2024

mmmavis commented Jan 25, 2024 •

edited by data-sync-user

Loading

mmmavis Jan 25, 2024 •

edited

Loading

mmmavis commented Jan 26, 2024 •

edited

Loading