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

Run Integration Tests on GitHub Actions #55

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

wba2hi
Copy link
Contributor

@wba2hi wba2hi commented Dec 1, 2023

Closes: #41

Closes: eclipse-kuksa#41
Signed-off-by: Andre Weber <andre.weber3@etas.com>
Copy link
Contributor

@Chrylo Chrylo left a comment

Choose a reason for hiding this comment

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

LGTM

  • Reports look good - Integration tests are executed


val dataEntry = capturingSlot.captured
val capturedDatapoint = dataEntry.value
val float = capturedDatapoint.float

assertEquals(newValue, float, 0.0001f)
}

`when`("The same value is set again") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I remember this was a system test, when executing "feed Vehicle.Speed 100" twice, the observer was only triggered once. But for some reason this behavior suddenly changed? Funnily this test still ran through successfully on friday.

Since it's no longer relevant I removed the test

@wba2hi
Copy link
Contributor Author

wba2hi commented Dec 4, 2023

I ran the build server a few times here to check for "sporadically failing tests": https://github.com/boschglobal/kuksa-android-sdk/actions/runs/7083450303

@@ -123,9 +121,10 @@ class DataBrokerSubscriberTest : BehaviorSpec({

and("When the FIELD_VALUE of Vehicle.Speed is updated") {
databrokerTransporter.updateRandomFloatValue(vssPath)
delay(100)
Copy link
Contributor Author

@wba2hi wba2hi Dec 4, 2023

Choose a reason for hiding this comment

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

since exactly = 0 is always fullfilled, the timeout does not apply here.

We therefore have to apply the delay because a) so that the verification is correctly "checked" after the asynchronously behavior would have run through and b) if we don't delay here the next test case in line 145ff is influenced,... (it fails because it receives three onPropertyChanged callbacks... => 1 from subscription, one from the update here and one from the update in the next testcase)

A bit unhappy with all the special edge cases we have for testing the DataBroker#subscribe functionality :(

Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

I'm not sure I get the exactly statements correct, but I think it should be anywhere -1 for my understanding. Please correct me if I'm wrong/understood your flow wrong. Otherwise the tests look good.

Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

LGTM

@wba2hi
Copy link
Contributor Author

wba2hi commented Dec 7, 2023

@SebastianSchildt
PR can be merged

@@ -11,6 +11,9 @@ jobs:
- name: Checkout code
uses: actions/checkout@v3

- name: "Run Docker Container of Databroker in detached mode"
run: docker run --pull=always --rm --publish 55556:55556/tcp --detach --name databroker ghcr.io/eclipse/kuksa.val/databroker:master --port 55556 --insecure

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine for this PR, but in the long run,

shall we test again "latest" (i.e. latest "release") or "Master" (bleeding edge) or fixed versions, or a combination thereof? wdyt @wba2hi @lukasmittag ?

Copy link
Contributor Author

@wba2hi wba2hi Dec 7, 2023

Choose a reason for hiding this comment

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

The best behavior would probably be to test against the "latest released version", so that we have a consistent behavior for us and "3rd parties" using our sdk and the corresponding databroker.

Edit: eclipse/kuksa.val#688 this already part of the latest release? Otherwise we get issues with #61 :D

Copy link
Contributor

@lukasmittag lukasmittag Dec 7, 2023

Choose a reason for hiding this comment

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

I think we should opt either for just master or a combination of master and latest. Because I think most of our documentation uses master and so we should test against it. For the future maybe we should change the documentation there too and then only test against latest (once we have a stable release process and release cycle time).

@SebastianSchildt SebastianSchildt merged commit 87f7fe9 into eclipse-kuksa:main Dec 7, 2023
5 checks passed
@erikbosch erikbosch deleted the feature-41 branch October 31, 2024 13:16
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.

Run Integration Tests on GitHub Actions
4 participants