Changes to set the ServerOfferedVersion and ServerOfferedHash always

[phanidevavarapu]

This PR will fix the bug #245, the ServerOfferedVersion and ServerOfferedHash are only set when the package is sent from the server the very first time.

The changes in this PR will make sure that the info is set all the time.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.96%. Comparing base (cec93de) to head (cc24fe9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   75.18%   75.96%   +0.78%     
==========================================
  Files          25       25              
  Lines        1656     1656              
==========================================
+ Hits         1245     1258      +13     
+ Misses        299      291       -8     
+ Partials      112      107       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[srikanthccv]

Please add a test case. Let's try to include cases one where the installation of the new version fails and another that succeeds.

[phanidevavarapu]

Please add a test case. Let's try to include cases one where the installation of the new version fails and another that succeeds.

@srikanthccv the unit test you suggested will cut across many methods with in this file itself, it will be a huge undertaking compared to the code change that I made.

[srikanthccv]

Regardless of the size of the change, we should add tests. That's how we prove the change actually fixes it and having a test ensures the issue will not appear again in future.

[tigrannajaryan]

Regardless of the size of the change, we should add tests. That's how we prove the change actually fixes it and having a test ensures the issue will not appear again in future.

+1. For all bug fixes we want tests that demonstrate that there is a bug (tests fail before the fix) and that the PR fixes the bug (tests pass after the fix).

I understand this adds more work, but this is important for maintaining the quality of the code and being confident about bug fixes.

[srikanthccv]

@phanidevavarapu I had opened a PR to add tests against your fork here phanidevavarapu#1. Did you get a chance to look at it?

[srikanthccv]

Can you fix the conflicts and update the packagessyncer.go because I commented out the lines to show tests fail.

[phanidevavarapu]

@srikanthccv updated the PR, There is a test failing, but I don't see any failures in local.

[tigrannajaryan]

TestOfferUpdatedVersion/ws test got stuck, likely an indication of a real problem.

[phanidevavarapu]

@tigrannajaryan I am not sure how to approach this now, tests seems to failing and passing at random. Any help to have everything green is really appreciated.

[srikanthccv]

While the individual test case go test -timeout 30s -run ^TestOfferUpdatedVersion$ github.com/open-telemetry/opamp-go/client passes successfully, running the entire test suite fails with a timeout failure consistently. We need to check why the collective test execution fails and fix it.

[phanidevavarapu]

Can we approve and merge this ticket?

[phanidevavarapu]

@srikanthccv @tigrannajaryan please look into this.

[srikanthccv]

The changes look good to me. I tried running the workflow multiple times to see if shows flaky behaviour but it passed all runs. I still suspect the test is flaky since it passes now without any changes to the test case.

[tigrannajaryan]

@phanidevavarapu this looks good to me, thank you. Can you please resolve the conflicts so that we can merge?

[phanidevavarapu]

@tigrannajaryan the merge conflicts are resolved, please go ahead and merge.