Skip to content

Add tags #737

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.

Sign up for GitHub

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

Jump to bottom
Open
sonic16x wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add tags #737

sonic16x wants to merge 10 commits into from

Conversation

@sonic16x
Copy link
Contributor

@sonic16x sonic16x commented Nov 27, 2025

No description provided.

@sonic16x sonic16x force-pushed the users/rocketraccoon/TESTPLANE-436.tags branch 5 times, most recently from 16528b9 to 08b77da Compare December 3, 2025 14:26
@github-actions
Copy link

github-actions bot commented Dec 3, 2025 •
edited
Loading

✅ Component tests succeed

Report

@github-actions
Copy link

github-actions bot commented Dec 3, 2025 •
edited
Loading

✅ E2E tests succeed

Report

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 4, 2025 •
edited
Loading

Open in StackBlitz

npm i https://pkg.pr.new/gemini-testing/html-reporter@737

commit: 9c590f6

@shadowusr
Copy link
Member

shadowusr commented Dec 4, 2025

I have a suggestion regarding tags visuals.

Before:
Screenshot 2025-12-05 at 1 17 12 AM

After:
Screenshot 2025-12-05 at 1 18 03 AM

Here's why I think the second variant is better:

  • It implement a long-standing feature request of having browser select on the test info card
  • We used to show browser as "tag", but it's just weird, it's not a tag, it's a browser
  • In the new variant, we don't have unncessary empty line for tags. I highly doubt anyone would utilise the whole line for that, I imagine users to have 1-2 tags max.
  • Streamlined design across tags and badges

Since I'm providing this feedback this late — which I am sorry for — I've implemented this redesign myself in this commit: 27ba464. Feel free to cherry pick. Should be mostly ready, you may check some edge cases like overflows by width, but other than that, should be all good.

If you have some suggestion regarding this redesign, I'd be glad to discuss them!

shadowusr
shadowusr requested changes Dec 4, 2025
View reviewed changes
Copy link
Member

@shadowusr shadowusr left a comment

Choose a reason for hiding this comment

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

A great additions, looking forward for this to be merged! 🔥

Really liked this contribution, and it's nice that you've implemented e2e tests for this feature! But I think it would be nice to fix aforementioned issues before proceeding.

lib/adapters/test-result/testplane/index.ts Outdated
return this._duration;
}

get tags(): TestTag[] {
Copy link
Member

@shadowusr shadowusr Dec 4, 2025

Choose a reason for hiding this comment

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

I don't see no reason for this to be a getter in this class, let alone public. All these adapters ideally should not implement anything beyond the ReporterTestResult interface.

I'd just move this logic to some extractTags helper and call it from attachments getter.

Copy link
Contributor Author

@sonic16x sonic16x Dec 5, 2025

Choose a reason for hiding this comment

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

Fixed

lib/adapters/test-result/testplane/index.ts
return list;
}

get attachments(): Attachment[] {
Copy link
Member

@shadowusr shadowusr Dec 4, 2025

Choose a reason for hiding this comment

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

How hard would it be to do the same in the playwright adapter? It would be really nice to have both tools working with tags and probably it's not too time consuming to implement

Copy link
Contributor Author

@sonic16x sonic16x Dec 5, 2025

Choose a reason for hiding this comment

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

It won't be very fast

lib/static/new-ui/features/suites/components/TestTags/index.tsx Outdated
tags.list.map((tag: TestTag) => (
<Tooltip
key={tag.title}
content={tag.dynamic && 'Dynamic tag'}
Copy link
Member

@shadowusr shadowusr Dec 4, 2025

Choose a reason for hiding this comment

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

IMO "Dynamic tag" doesn't explain what exactly is different with this tag. I'd leave a more detailed note, something along the lines of:

Tag was added via browser.addTag() command. Such tags can't be used with --tag filtering in CLI.

Copy link
Contributor Author

@sonic16x sonic16x Dec 5, 2025

Choose a reason for hiding this comment

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

Fixed

@sonic16x sonic16x force-pushed the users/rocketraccoon/TESTPLANE-436.tags branch from fad7b13 to fc88019 Compare December 5, 2025 05:44
@sonic16x sonic16x force-pushed the users/rocketraccoon/TESTPLANE-436.tags branch from fc88019 to a3c8db9 Compare December 5, 2025 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shadowusr shadowusr shadowusr requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sonic16x @shadowusr