-
Notifications
You must be signed in to change notification settings - Fork 73
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
Address GitHub CI issues #5465
Address GitHub CI issues #5465
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides Run #10989
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5465/merge
|
Run status |
Passed #10989
|
Run duration | 00m 37s |
Commit |
87bc0b7332 ℹ️: Merge 8617dfcc9745530125636d458b91b32ebff739d5 into aa9155af32f1885f831006010eee...
|
Committer | Jason Gill |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5465 +/- ##
=======================================
Coverage 85.47% 85.47%
=======================================
Files 384 384
Lines 24119 24119
Branches 2625 2625
=======================================
Hits 20615 20615
Misses 2950 2950
Partials 554 554 ☔ View full report in Codecov by Sentry. |
00f6df7
to
70911eb
Compare
7f4dcd5
to
7ba9f9a
Compare
335cebe
to
6af41f1
Compare
6af41f1
to
ee68312
Compare
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.
Great work Jason! We can now get into the habit of making tests that are fully autonomous. I have ran every test without the backend running and they're all passing.
I left a question about a change I don't understand.
}, | ||
).as("postGenerate"); | ||
cy.intercept("POST", "/api/v1/generate", { | ||
delay: 1000, |
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.
Nice cleanup
@@ -79,7 +81,7 @@ describe("Data map report table", () => { | |||
|
|||
it("can render empty datamap report", () => { | |||
cy.intercept("GET", "/api/v1/plus/datamap/minimal*", { | |||
body: { items: [], page: 1, pages: 0, size: 25, total: 0 }, | |||
fixture: "empty-pagination.json", |
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.
very reusable
fixture: "privacy-experiences/experienceConfig.json", | ||
}).as("getExperienceDetail"); | ||
stubPlus(true); | ||
stubProperties(); |
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.
We had so many missing stubs, thanks for taking care of this.
if (!response.ok) { | ||
let response: Response; | ||
|
||
try { |
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.
Nice improvement on error handleling
transformResponse: ( | ||
response: CustomFieldDefinitionWithId[] | { detail: string }, | ||
) => { | ||
if ("detail" in response) { |
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.
I don't get the context for this change. Could you explain?
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.
That's a great question that I'm trying to investigate and remember...
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.
@lucanovera I updated this code with a comment to explain why. For some reason, instead of an error code or an empty list, the BE is returning { detail: "No custom metadata fields found with resource type system" } which was causing a console error in this original piece of code.
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 for the research! It's definitely a weird response by the BE, not a pattern we usually follow. I've created a tech debt ticket for it (https://ethyca.atlassian.net/browse/LA-142). After that is done, we won't need this custom handling. But for now, this is great.
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 idea! Thanks for doing that.
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.
Great work! All questions cleared, all tests passing. Approved!
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.
Everything is passing for me-- great job cleaning all this up.
body: { access_result_urls: ["https://example.com/"] }, | ||
}).as("getAccessResultURL"); | ||
cy.wait("@getPrivacyRequest"); | ||
stubPrivacyRequests(PrivacyRequestStatus.COMPLETE); | ||
// cy.wait("@getPrivacyRequest"); |
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.
Can this be removed?
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 good and passing all tests for me.
fides Run #10990
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #10990
|
Run duration | 00m 38s |
Commit |
d14d031e3d: Address GitHub CI issues (#5465)
|
Committer | Jason Gill |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Cypress tests are inconsistent, randomly fail, are quite slow. To help address this, lets:
cypress-io/github-action
to v6ECONNREFUSED
errors with no BE running (/api/v1/system/
,/health
, etc)