Skip to content

re-wrote tests for methods copy and extractSubstructure based on Hamc…#895

Merged
johnmay merged 1 commit intocdk:extraSubstructurefrom
uli-f:extraSubstructure
Sep 9, 2022
Merged

re-wrote tests for methods copy and extractSubstructure based on Hamc…#895
johnmay merged 1 commit intocdk:extraSubstructurefrom
uli-f:extraSubstructure

Conversation

@uli-f
Copy link
Copy Markdown
Member

@uli-f uli-f commented Sep 9, 2022

…rest's assertThat

@uli-f uli-f mentioned this pull request Sep 9, 2022
Merged
@egonw egonw self-requested a review September 9, 2022 05:05
egonw
egonw approved these changes Sep 9, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@egonw egonw left a comment

Choose a reason for hiding this comment

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

I scrolled through it, and the restructuring is clear and I did not spot transcription errors.

@egonw
Copy link
Copy Markdown
Member

egonw commented Sep 9, 2022

@johnmay, am I blind, or did the tests not run on this? I don't see the test/coverage reports...

@uli-f
Copy link
Copy Markdown
Member Author

uli-f commented Sep 9, 2022

@egonw Nope, tests didn't run 🤷🏼

@egonw
Copy link
Copy Markdown
Member

egonw commented Sep 9, 2022

@egonw Nope, tests didn't run 🤷🏼

I cannot find how to manually get it started. Normally there is an option to run it. But not here. Maybe because this is a PR on a PR?

@egonw
Copy link
Copy Markdown
Member

egonw commented Sep 9, 2022

I tested it locally with Java11 and that does not give failing tests.

@uli-f
Copy link
Copy Markdown
Member Author

uli-f commented Sep 9, 2022

I cannot find how to manually get it started. Normally there is an option to run it. But not here. Maybe because this is a PR on a PR?

Yep, that seems like a good guess to me. Looking at #891 and #893 there aren't any checks available either.

@uli-f
Copy link
Copy Markdown
Member Author

uli-f commented Sep 9, 2022

I tested it locally with Java11 and that does not give failing tests.

I did the same on Java 17, all good 🟢

@johnmay
Copy link
Copy Markdown
Member

johnmay commented Sep 9, 2022

Could be the permissions changes to the workflow that were suggested.

@johnmay
Copy link
Copy Markdown
Member

johnmay commented Sep 9, 2022

Or maybe it's because this is a PR into another PR.

@johnmay
Copy link
Copy Markdown
Member

johnmay commented Sep 9, 2022

The assertThat was just a suggestion BTW - didn't expect to use it :-)

@johnmay johnmay merged commit 0f56904 into cdk:extraSubstructure Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@egonw egonw egonw approved these changes

Assignees

@johnmay johnmay

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@uli-f @egonw @johnmay