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 released versions as well #63

Closed
SebastianSchildt opened this issue Dec 7, 2023 · 6 comments
Closed

Run integration tests on released versions as well #63

SebastianSchildt opened this issue Dec 7, 2023 · 6 comments

Comments

@SebastianSchildt
Copy link
Contributor

See #55 for discussion.

Currently CI runs against databroker:master

That is good to check whether we (or databroker) did break something, but we should additionally at lest also test aginst ":latest" which is the latest release, as this is what people should/could expect to work

@wba2hi
Copy link
Contributor

wba2hi commented Feb 2, 2024

@SebastianSchildt
Any idea how we want to deal with the scenario when a test runs successful against one branch (e.g. databroker:master) but not the other branch (e.g. databroker:latest) because a breaking change or behavioral change was introduced? This could lead to a stale situation where we are not able to merge a PR due to failing tests.

Like here: https://github.com/eclipse-kuksa/kuksa-android-sdk/pull/55/files/79eb2c0c730127ce5581ea9134fd134f8e1af875#r1413518058

Fixing this behavior works against master or against latest but it can't be "fixed" for both :D

We could of course remove the tests as we did here but I don't think that's the best solution :D

@SebastianSchildt
Copy link
Contributor Author

This is a hard question. First idea:

Could we configure it as such, that latest ins mandatory but the "master" test can not block the merge? That way whatever you do, defintely shoudl work with the latest released version, and in the optimal case towards master as well, i.e. when introducting a breaking change in databroker, with the intent to release, you better make sure master works as well. I know this has corner cases, and in case of intended breaking changes might need to have two code paths in the library at some point for backwards compatibility, but it still seems like a reasonable approach, until it fails us :)

I certain cases (assume we ADD functionality) we might need to make test cases a bit more intelligent, that they check which version they run agains, and skip tests of fancy new features when running agains an older version

@wba2hi
Copy link
Contributor

wba2hi commented Feb 5, 2024

Could we configure it as such, that latest ins mandatory but the "master" test can not block the merge?

While building the app on GitLab I saw some tasks marked as "allowed-to-fail". If there is some similar functionality on GitHub it should be a no-brainer. A short google for it (actions/runner#2347), shows that there only seems to be "continue-on-error", which will simply mark the failing task green / as succeeded and therefore hides the real result of the test. Might be possible, but probably takes some time of try-and-error and fiddling around to make it work.

I certain cases (assume we ADD functionality) we might need to make test cases a bit more intelligent, that they check which version they run agains, and skip tests of fancy new features when running agains an older version

Yes, this sounds like the correct way to do it, however for this we need to know the version of the Databroker from the Databroker itself (on connection, via separate api, ...). Is this already possible or something which needs to be implemented on Databroker side?

@SebastianSchildt
Copy link
Contributor Author

Yes, this sounds like the correct way to do it, however for this we need to know the version of the Databroker from the Databroker itself (on connection, via separate api, ...). Is this already possible or something which needs to be implemented on Databroker side?

In recent releases (0.4.1) Kuksa.Databroker.CargoVersion might be used, if that is 404 it is an older release

     ⢀⣤⣶⣾⣿⢸⣿⣿⣷⣶⣤⡀
    ⣴⣿⡿⠋⣿⣿   ⠈⠙⢿⣿⣦
   ⣾⣿⠋  ⣿⣿  ⣶⣿  ⠙⣿⣷
  ⣸⣿⠇   ⣿⣿⠠⣾⡿⠃   ⠸⣿⣇  ⣶ ⣠⡶⠂ ⣶  ⢰⡆ ⢰⡆⢀⣴⠖ ⢠⡶⠶⠶⡦   ⣰⣶⡀
  ⣿⣿    ⠿⢿⣷⣦⡀     ⣿⣿  ⣿⢾⣏   ⣿  ⢸⡇ ⢸⡷⣿⡁  ⠘⠷⠶⠶⣦  ⢠⡟⠘⣷
  ⢹⣿⡆   ⣿⣶⠈⢻⣿⡆   ⢰⣿⡏  ⠿ ⠙⠷⠄ ⠙⠷⠶⠟⠁ ⠸⠇⠈⠻⠦ ⠐⠷⠶⠶⠟ ⠠⠿⠁ ⠹⠧
   ⢿⣿⣄  ⣿⣿  ⠿⣿  ⣠⣿⡿
    ⠻⣿⣷⡄⣿⣿   ⢀⣠⣾⣿⠟    kuksa-client CLI
     ⠈⠛⠇⢿⣿⣿⣿⣿⡿⠿⠛⠁     0.4.2

Default tokens directory: /kuksa-client/_internal/kuksa_certificates/jwt

Connecting to VSS server at 127.0.0.1 port 55556 using KUKSA GRPC protocol.
TLS will not be used.
INFO 2024-02-05 12:46:41,182 kuksa_client.grpc No Root CA present, it will not be possible to use a secure connection!
INFO 2024-02-05 12:46:41,182 kuksa_client.grpc.aio Establishing insecure channel
gRPC channel connected.
Test Client> getValue Kuksa.Databroker 
[
    {
        "path": "Kuksa.Databroker.CargoVersion",
        "value": {
            "value": "0.4.1",
            "timestamp": "2024-02-05T12:45:41.625285+00:00"
        }
    },
    {
        "path": "Kuksa.Databroker.CommitSha",
        "value": {
            "value": "6690ca970a2f7dd8245bd00f6b10788eaad755b5",
            "timestamp": "2024-02-05T12:45:41.625286+00:00"
        }
    },
    {
        "path": "Kuksa.Databroker.GitVersion",
        "value": {
            "value": "N/A",
            "timestamp": "2024-02-05T12:45:41.625270+00:00"
        }
    }
]

Test Client> 

@Chrylo
Copy link
Contributor

Chrylo commented Feb 7, 2024

To just throw this question in here:

First of all: Does it make sense to adapt our PRs to test against databroker:latest instead of master? Because like @SebastianSchildt said this is what people actually use. With this in mind:

Not sure if we need this really on a PR level. Because it feels more like a warning system that the current unstable "master:Databroker" is not compatible with our main:SDK or broke something. I agree this would be interesting to see this warning on our "SDK:main" branch but on PR it feels a bit off because one day the PR may be green and on a re-run it shows a warning. As soon as we know that the current "SDK:main" throws a warning we could create a ticket to adapt the SDK to breaking changes. But this would normally not happen in the current PR because the change will be out of scope most likely and only confuses the PR developer.

So the proposed solution:

  • SDK:latest will be tested against databroker:main + databroker:latest on a daily / weekly basis
  • SDK:main will be tested against databroker:main on a daily / weekly basis
  • SDK:PR will be tested against databroker:latest

My 2 cents. This also fixes the mentioned issue.

@Chrylo
Copy link
Contributor

Chrylo commented Feb 27, 2024

Tickets were created. This discussion can be closed. FYI @SebastianSchildt

@Chrylo Chrylo closed this as completed Feb 27, 2024
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

No branches or pull requests

3 participants