Skip to content

E2E: QA: add acceptance tests for approved color label#23046

Open
andr317c wants to merge 5 commits into
mainfrom
v18/QA/add-acceptance-tests-for-color-picker
Open

E2E: QA: add acceptance tests for approved color label#23046
andr317c wants to merge 5 commits into
mainfrom
v18/QA/add-acceptance-tests-for-color-picker

Conversation

@andr317c
Copy link
Copy Markdown
Contributor

@andr317c andr317c commented Jun 2, 2026

No description provided.

Copilot AI review requested due to automatic review settings June 2, 2026 18:41
@claude
Copy link
Copy Markdown

claude Bot commented Jun 2, 2026

Claude finished @andr317c's task in 2m 57s —— View job


Code Review

  • Gather context and read changed files
  • Review DataTypeApiHelper.ts changes
  • Review ContentWithApprovedColorLabel.spec.ts
  • Check against existing acceptance test patterns
  • Post review findings
  • Apply labels (area/test, category/test-automation)

Good addition — the two tests cover a meaningful regression scenario (label on content being updated to reflect a data type label change after re-publish), and the overall structure follows established patterns in the test suite. A few small points worth addressing:


DataTypeApiHelper.tsupdateApprovedColorItemLabel

[Low] Missing optional chain on .value.find()

const colorItem = itemsValue?.value.find(item => item.value === color);

If itemsValue is undefined (i.e. the data type has no items value configured), itemsValue?.value short-circuits to undefined, but the subsequent .find(...) call without optional chaining will throw a TypeError. The fix is:

const colorItem = itemsValue?.value?.find(item => item.value === color);

Note: the pre-existing doesApprovedColorHaveColor method in the same file has the identical pattern — this isn't new behaviour, but worth fixing here since the method is being introduced in this PR. Fix this →

[Low] Silent no-op when color is not found

If colorItem is undefined (color value doesn't match any item), the method silently calls this.update() with unchanged data — making an unnecessary API request and potentially masking a bug in a test. Consider throwing or returning early when no match is found:

if (!colorItem) {
  throw new Error(`Color '${color}' not found in data type '${dataTypeName}'`);
}

ContentWithApprovedColorLabel.spec.ts

[Low] Variable name contentId should be contentKey

The return value of createPublishedDocumentWithValue is a UUID key (the API location header), not a numeric ID. Existing similar tests (e.g. RenderingContentWithApprovedColor.spec.ts:9) name this contentKey. Renaming contentIdcontentKey at lines 12, 17, and 52 would be more consistent. Fix this →

[Positive] Pre-condition assertion in test 1 is well-structured

The check at line 33 (expect(oldContentData.values[0].value.label).toBe(oldLabel)) correctly validates that the stale label persists in saved content before re-publishing. This is exactly the invariant that matters for this bug scenario.

[Positive] isEqual: true in doesContentRenderValueContainText

Using the strict equality assertion (true) in the render test is appropriate — we want to confirm the exact label text is present on the page, not just a substring match.


Verdict: ✅ Approve with suggestions

No blocking issues. The two low-severity findings are easy to address but won't affect test correctness in practice (the data type is always created with items before updateApprovedColorItemLabel is called). The tests are well-structured, clean, and cover the intended scenario.

andr317c added 2 commits June 2, 2026 20:42
…color-picker' into v18/QA/add-acceptance-tests-for-color-picker
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds end-to-end coverage ensuring Approved Color values remain consistent when an Approved Color data type’s item label is changed, and that republishing updates both stored value metadata and rendered output.

Changes:

  • Adds a new acceptance spec verifying label updates are applied to existing content after republish.
  • Adds a rendered-content assertion to ensure the updated label is displayed on the frontend after republish.
  • Extends the acceptance test DataType API helper with a method to update an Approved Color item label.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/Umbraco.Tests.AcceptanceTest/tests/DefaultConfig/Content/ContentWithApprovedColorLabel.spec.ts New E2E tests for updating and rendering Approved Color labels after data type label changes.
tests/Umbraco.Tests.AcceptanceTest/lib/helpers/DataTypeApiHelper.ts Adds helper API to update an Approved Color item’s label to support the new E2E scenarios.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 3, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants