Skip to content

UPSE-386: Update Playwright search tests for 'favorite' flag - #648

Open
groybal wants to merge 1 commit into
uPortal-Project:masterfrom
groybal:UPSE-386
Open

groybal wants to merge 1 commit into
uPortal-Project:masterfrom
groybal:UPSE-386

Conversation

@groybal

@groybal groybal commented Aug 1, 2023

Copy link
Copy Markdown
Contributor
Checklist
Description of change

For my uPortal PR (uPortal-Project/uPortal#2678) for UPSE-386, it was requested that I update the Playwright scripts to account for the 'favorite' flag added to the portlet results. This PR does that.

This PR should not be merged until the uPortal UPSE-386 PR is merged.

@sonatype-lift

sonatype-lift Bot commented Aug 1, 2023

Copy link
Copy Markdown

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@cbeach47 cbeach47 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor inline comment.

Overall, LTGM - thanks @groybal !

);
expect(response.status()).toEqual(200);
const responseBody: PreferenceFavoriteResponse = JSON.parse(await response.text()) as PreferenceFavoriteResponse;
return responseBody.newNodeId;//response.startsWith('Added');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like we could remove this comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, I'll remove

@cbeach47

cbeach47 commented Aug 1, 2023

Copy link
Copy Markdown
Contributor

@groybal - for

This PR should not be merged until the uPortal UPSE-386 PR is merged.

This PR should actually not be merged until that PR is merged AND a release is cut. Current phase of these playwright tests work off of the uPortal-start configs, which are the releases for the core uPortal and portlet services. I'd like to see how we can enhance this flow so the playwright tests 'go with' the code change for a given repo.

@ChristianMurphy ChristianMurphy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @groybal! LGTM
Added some nice to have ideas for the PR.

Comment thread tests/api/search-v5_0.spec.ts Outdated
const response = await request.get(`${config.url}api/v5-0/portal/search?q=cartoon&type=portlets`);
expect(await unfavoritePortlet(request, portletId)).toBe(true);
expect(response.status()).toEqual(200);
const portletSearchResults: PortletSearchResults = JSON.parse(await response.text()) as PortletSearchResults;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

an as cast is probably fine for a test case.
Using a JSON parser/validator like zod would give better assurance that the structure returns matches the test expectation. https://zod.dev/

Comment thread tests/api/utils/api-portlets-utils.ts Outdated
Comment thread tests/api/utils/api-portlets-utils.ts Outdated
Comment thread tests/api/utils/api-portlets-utils.ts Outdated
): Promise<PortletDefBasicInfo> {
const response = await request.get(`${config.url}api/portlets.json`);
expect(response.status()).toEqual(200);
const portletsJson: PortletsResponse = await response.json() as PortletsResponse;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it would be nice if we parsed/validated the JSON.

`${config.url}api/layout?action=addFavorite&channelId=${portletId}`
);
expect(response.status()).toEqual(200);
const responseBody: PreferenceFavoriteResponse = JSON.parse(await response.text()) as PreferenceFavoriteResponse;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it would be nice if we parsed/validated the JSON.

expect(portletId).not.toBeNull();
const response = await request.post(`${config.url}api/layout?action=removeFavorite&channelId=${portletId}`);
expect(response.status()).toEqual(200);
const responseBody: PreferenceFavoriteResponse = JSON.parse(await response.text()) as PreferenceFavoriteResponse;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it would be nice if we parsed/validated the JSON.

Comment on lines +13 to +25
interface PortletSearchResult {
description: string;
fname: string;
name: string;
score: string;
title: string;
url: string;
favorite: boolean;
}

interface PortletSearchResults {
portlets: PortletSearchResult[];
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Completely optional, but if you are interested in trying Zod.
This would look something like:

import z from 'zod';

const PortletSearchResultSchema = z.object({
  description: z.string(),
  fname: z.string(),
  name: z.string(),
  score: z.string(),
  title: z.string(),
  url: z.string(),
  favorite: z.boolean();
});
type PortletSearchResult = z.infer<typeof PortletSearchResultSchema>;

const PortletSearchResultsSchema = z.object({
  portlets: z.array(PortletSearchResultSchema)
});
type PortletSearchResults = z.infer<typeof PortletSearchResultsSchema>

this could later be used:

const portletSearchResults: PortletSearchResults = PortletSearchResultsSchema.parse(JSON.parse(await response.text()));

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.

4 participants