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

chore: End a/b campaigns for aci smart banners #25504

Merged
merged 7 commits into from
Jan 25, 2023

Conversation

mike-plummer
Copy link
Contributor

User facing changelog

Standardizes language within in-app banners that assist users with connecting projects to the Cypress Cloud.

Additional details

Ending A/B testing campaigns for ACI project connection banners

Steps to test

Should be covered by existing tests

How has the user experience changed?

No user visible change - three banners will simply always show a chosen version of content that was already being displayed to a subset.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@cypress
Copy link

cypress bot commented Jan 18, 2023

46 flaky tests on run #43426 ↗︎

0 26600 1274 0 Flakiness 46

Details:

Merge branch 'develop' into mikep/24472-end-smart-banner-campaigns
Project: cypress Commit: ed04c4bc6e
Status: Passed Duration: 18:47 💡
Started: Jan 25, 2023 4:36 PM Ended: Jan 25, 2023 4:55 PM
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test
network stubbing > intercepting request > can delay and throttle a StaticResponse
Flakiness  e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test
cy.origin waiting > alias > waits for the route alias to have a response
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry
Flakiness  e2e/origin/cookie_behavior.cy.ts • 4 flaky tests • 5x-driver-electron

View Output Video

Test
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
Flakiness  e2e/origin/commands/navigation.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test
cy.origin navigation > #consoleProps > .go()

The first 5 failed specs are shown, see all 0 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@mike-plummer mike-plummer marked this pull request as ready for review January 18, 2023 21:11
@mike-plummer mike-plummer requested review from warrensplayer and a team January 18, 2023 21:12
cohortOptionSelected.value = config.options.find((option) => option.cohort === cohortSelected.data?.determineCohort?.cohort)
}
const fetchCohort = async () => {
const cohortSelected = await determineCohort(name, cohortIds)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to determine the cohort? Isn't there only 1 cohort now that the A/B testing ended? Is this now dead code or a no-op?

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 we should add a comment that the infrastructure is left is place as future a/b testing is expected within this same place. I recall asking this somewhere in the original issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, since there are no active experiments this is currently dead code. However, product wants the ability to quickly introduce new experiments so it was requested that the logic be maintained. I will add some comments to this area to clarify

@lmiller1990
Copy link
Contributor

Looks good to me!

@astone123 astone123 self-requested a review January 23, 2023 22:25
Copy link
Contributor

@astone123 astone123 left a comment

Choose a reason for hiding this comment

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

There are some questionable changes in the Percy build here. It looks like some specs list tests are showing banners when they didn't before. It's not clear to me why this is happening from looking at the diff, but we should check them out before merging.

I marked the problematic snapshot changes in the Percy build

@lmiller1990
Copy link
Contributor

@astone123 good catch... if this is a regression, we should also add a Cypress assertion as well - Percy is good, but I don't think it should be the only test, especially when it's easy do so a cy.get('data-cy="banner"') style test.

@astone123 astone123 self-requested a review January 25, 2023 16:02
Copy link
Contributor

@astone123 astone123 left a comment

Choose a reason for hiding this comment

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

After talking to Mike about this, the changes that I pointed out in the Percy snapshots weren't regressions, just a result of the cohorts being immediately resolved, so the banners show up during those tests when they didn't before.

We have other tests for ensuring banners only appear when they're supposed to, so I think it's fine to just update the snapshots. Looks good 👍🏻

@mike-plummer mike-plummer merged commit f1f2c73 into develop Jan 25, 2023
@mike-plummer mike-plummer deleted the mikep/24472-end-smart-banner-campaigns branch January 25, 2023 17:53
tgriesser added a commit that referenced this pull request Jan 26, 2023
* develop: (27 commits)
  refactor: remove unused cloud routes (#25584)
  chore: fix issue template formatting issue (#25587)
  fix: fixed issue with CT + electron + run mode not exiting properly (#25585)
  chore(deps): update dependency ua-parser-js to v0.7.33 [security] (#25561)
  fix: add alternative binary names for edge-beta (#25456)
  chore: add batch execution to CloudDataSource (#22457)
  chore: End a/b campaigns for aci smart banners (#25504)
  chore: release @cypress/schematic-v2.5.0
  fix(cypress-schematic): do not disable e2e support file (#25400)
  chore: adding memory issue template (#25559)
  feat: Add Angular CT Schematic (#24374)
  chore: enforce changelog entries on PR reviews (#25459)
  chore: bump package.json to 12.4.0 [run ci] (#25556)
  feat: Add 'type' option to `.as` to store aliases by value (#25251)
  chore: release @cypress/webpack-dev-server-v3.2.3
  feat: Display line break in cy.log (#25467)
  chore: update types (#25538)
  fix: Revert "fix: adding emergency garbage collection for chromium-based browsers" (#25546)
  fix: percy - wait to take snapshot until previous tooltips are gone (#25522)
  feat: support data-qa selector in selector playground (#25475)
  ...
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 27, 2023

Released in 12.4.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.4.1, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conclude Smart Prompt A/B tests
4 participants