Skip to content
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

chore(upgrades): Update OS to latest supported releases #687

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

jasonmcintosh
Copy link
Member

No description provided.

resources.filter {
subject.apply(it, emptyList()) == null
}.let { filteredResources ->
filteredResources.size shouldMatch equalTo(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this failed with:

AmazonTagExclusionPolicyTest > should handle high precision date times() FAILED
    java.lang.AssertionError: expected: a value that is equal to 1
    but was: 2
        at com.natpryce.hamkrest.assertion.Assertion.that(assert.kt:17)
        at com.natpryce.hamkrest.assertion.Assertion.that$default(assert.kt:14)
        at com.natpryce.hamkrest.assertion.Assert.assertThat(assert.kt:45)
        at com.natpryce.hamkrest.should.Should.shouldMatch(should.kt:15)
        at com.netflix.spinnaker.swabbie.aws.exclusions.AmazonTagExclusionPolicyTest.should handle high precision date times(AmazonTagExclusionPolicyTest.kt:160)

but it doesn't seem correct to totally remove the assertions. Do you know why it failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's because i copied & pasted the above test & realized today that test seemed to depend upon the date comparison. AKA the test was just a copy/paste to test serialization of the date string but I included all the OTHER logic without really looking at it so i stripped it all out JUST for the serialization aspects.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this test needs at least one assertion, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically no ;) All we're doing is testing the serialization - assertions are NOT technically required particularly if an exception is expected on an operation. E.g. this is... almost just testing the constructor behavior passes without fialures. I can add an assertion if we really want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@dbyron-sf dbyron-sf added the ready to merge Reviewed and ready for merge label Nov 19, 2024
@mergify mergify bot added the auto merged label Nov 19, 2024
@mergify mergify bot merged commit 609df99 into spinnaker:master Nov 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged ready to merge Reviewed and ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants