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

FDP-2359: command handling reboot #39

Merged
merged 58 commits into from
Sep 10, 2024
Merged

FDP-2359: command handling reboot #39

merged 58 commits into from
Sep 10, 2024

Conversation

loesimmens
Copy link
Contributor

No description provided.

jasperkamerling and others added 17 commits August 26, 2024 16:47
Signed-off-by: Jasper Kamerling <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Jasper Kamerling <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Comment on lines 80 to 81
val parts = commandInProgress.type.parts
return parts.all { part -> downlink.contains(part) }
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to use regex pattern matchers instead of parts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored a bit, but without regex. Do you have a regex suggestion that improves on this?

loesimmens and others added 18 commits September 4, 2024 15:24
Signed-off-by: Loes Immens <[email protected]>
…/service/CommandServiceTest.kt

Co-authored-by: Sander van der Heijden <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Sander Verbruggen <[email protected]>
@@ -190,7 +189,7 @@ class CoapMessageHandlingTest {
}

@Test
fun shouldSendCommandToProxyAndSaveWithStatusInProgress() {
fun shouldSaveCommandWithStatusPendingAndSendReceivedFeedbackWhenReceivingCommandFromMaki() {
Copy link
Member

Choose a reason for hiding this comment

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

This test method should probably be moved to a different class, as it has nothing to do with handling coap messages?

Comment on lines 15 to 18
const val DEVICE_ID = "device-id"
const val MESSAGE_RECEIVED = "Command received"
val CORRELATION_ID = UUID.randomUUID()
val timestamp = Instant.now()
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a separate TestConstants class for the shared test constants.

Comment on lines 33 to 37
val expectedException = CommandValidationException("Command unknown")

assertThatThrownBy { CommandMapper.externalCommandToCommandEntity(externalCommand, status) }
.usingRecursiveComparison()
.isEqualTo(expectedException)
Copy link
Member

Choose a reason for hiding this comment

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

I think I might prefer one of these possible alternatives without using recursive comparison:

  • with checks on type and message:
        assertThatThrownBy { CommandMapper.externalCommandToCommandEntity(externalCommand, status) }
            .isInstanceOf(CommandValidationException::class.java)
            .hasMessage("Command unknown: ${externalCommand.command}")
  • with (likely) increased readability:
        assertThatExceptionOfType(CommandValidationException::class.java)
            .isThrownBy { CommandMapper.externalCommandToCommandEntity(externalCommand, status) }
            .withMessage("Command unknown: ${externalCommand.command}")
  • with a clearer distinction between "act" and "assert":
        val actual = catchThrowable {
            CommandMapper.externalCommandToCommandEntity(externalCommand, status)
        }

        assertThat(actual)
            .isInstanceOf(CommandValidationException::class.java)
            .hasMessage("Command unknown: ${externalCommand.command}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mooier inderdaad

Comment on lines 50 to 53
val command = "invalid"

assertThat(result).isEqualTo("REBOOT")
assertThatThrownBy { CommandMapper.commandNameToType(command) }
.isInstanceOf(IllegalArgumentException::class.java)
Copy link
Member

Choose a reason for hiding this comment

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

You could even reduce this to:

        assertThatIllegalArgumentException().isThrownBy {
            CommandMapper.commandNameToType("invalid")
        }

or more verbose with a clearer distinction between the "triple A" sections ("arrange, "act", and "assert")

    val commandName = "invalid"

    val actual = catchThrowable { CommandMapper.commandNameToType(commandName) }

    assertThat(actual).isInstanceOf(IllegalArgumentException::class.java)
  

loesimmens and others added 2 commits September 10, 2024 15:20
…/consumer/CommandConsumerTest.kt

Co-authored-by: Sander van der Heijden <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Signed-off-by: Loes Immens <[email protected]>
Copy link

sonarcloud bot commented Sep 10, 2024

@loesimmens loesimmens merged commit 494ec81 into main Sep 10, 2024
3 checks passed
@loesimmens loesimmens deleted the feature/FDP-2359 branch September 10, 2024 14:40
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