-
Notifications
You must be signed in to change notification settings - Fork 52
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
UPSE-386: Update Playwright search tests for 'favorite' flag #648
base: master
Are you sure you want to change the base?
Conversation
Sonatype Lift is retiringSonatype 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. |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@groybal - for
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. |
There was a problem hiding this comment.
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.
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; |
There was a problem hiding this comment.
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/
): 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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
interface PortletSearchResult { | ||
description: string; | ||
fname: string; | ||
name: string; | ||
score: string; | ||
title: string; | ||
url: string; | ||
favorite: boolean; | ||
} | ||
|
||
interface PortletSearchResults { | ||
portlets: PortletSearchResult[]; | ||
} |
There was a problem hiding this comment.
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()));
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.