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

Skip failing custom dc tests. #4820

Merged
merged 1 commit into from
Dec 27, 2024
Merged

Conversation

keyurva
Copy link
Contributor

@keyurva keyurva commented Dec 27, 2024

  • Certain place explorer tests are failing on custom DC autopush. This PR skips those tests for now.
  • The failures are likely due to recent place explorer page changes that have probably not been ported to custom DC.
  • Filed b/386405593 to fix and unskip them subsequently.
  • Also filed b/386405338 to run these tests on each PR so they are caught at the outset.

@keyurva keyurva requested a review from clincoln8 December 27, 2024 22:30
@keyurva keyurva enabled auto-merge (squash) December 27, 2024 22:38
from server.webdriver.shared_tests.place_explorer_i18n_test import \
PlaceI18nExplorerTestMixin


class TestPlaceExplorerI18n(PlaceI18nExplorerTestMixin, CdcAutopushTestBase):
"""Class to test the i18n place explorer page for Custom DC. Tests come from PlaceI18nExplorerTestMixin."""

@unittest.skip(reason=SKIPPED_TEST_REASON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick note: we already have an approach for skipping unit tests in custom DC. You can remove the test from the shared_tests mixin and add it directly to the main DC /tests/ package instead. Here's an example example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the note. In these 2 cases I don't know if we actually want to remove them or fix and keep them in custom DC. I'll work with Gabe in the new year to figure it out.

Skipping them explicitly for now to stop the alerts until then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what is done here is better because if this test passes on base dc we should keep it! We definitely should fix this test for custom dc. I like the way keyur did it here!
Also I think we ve had enough instances of breaking auto push cdc with these webdriver tests. We should really try to have these run on a local cdc as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good—skipping for now makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

@gmechali agreed. is testing against autopush helpful, or should these tests only run against a local cdc?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue it's still useful since it allows us to test what's deployed in autopush and catch bugs pre releases
But yeah local would probably be a bit more valuable since we d catch it even earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks both for chiming in!

@keyurva keyurva merged commit c8a8c66 into datacommonsorg:master Dec 27, 2024
9 checks passed
@keyurva keyurva deleted the ignore-tests branch December 27, 2024 22:50
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.

3 participants