search

opiproject / opi-evpn-bridge

OPI gRPC to EVPN GW FRR bridge
Apache License 2.0
14 stars 13 forks source link

fix(evpn-bridge): fix system behaviour for pending objects #391

Open mardim91 opened 3 months ago

mardim91 commented 3 months ago

This is a fix related to the issue https://github.com/opiproject/opi-evpn-bridge/issues/388

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 3.70370% with 52 lines in your changes missing coverage. Please review.

Project coverage is 24.44%. Comparing base (5440fb9) to head (85bde59). Report is 20 commits behind head on main.

Files Patch % Lines
pkg/infradb/taskmanager/taskmanager.go 2.38% 41 Missing :warning:
...g/infradb/subscriberframework/eventbus/eventbus.go 8.33% 11 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #391 +/- ## =========================================== - Coverage 50.77% 24.44% -26.33% =========================================== Files 37 44 +7 Lines 2525 5674 +3149 =========================================== + Hits 1282 1387 +105 - Misses 1114 4120 +3006 - Partials 129 167 +38 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mardim91 commented 3 months ago

I am wondering if we could create some unit tests which could prove that this approach work and free of race conditions. Go tests can be run with -race flag to catch any race conditions: https://go.dev/doc/articles/race_detector

I have tested this code locally in order to check if the different corner cases that are resolved by this code here are working properly. Everything was working as it should be.

The unit tests would be a very usefull tool to add in order not to test all the cases by hand every time but right now I do not have time to work on that so I will propose to open an issue so we can address into the future when I find some time if that is allright.

artek-koltun commented 3 months ago

I am wondering if we could create some unit tests which could prove that this approach work and free of race conditions. Go tests can be run with -race flag to catch any race conditions: https://go.dev/doc/articles/race_detector

I have tested this code locally in order to check if the different corner cases that are resolved by this code here are working properly. Everything was working as it should be.

The unit tests would be a very usefull tool to add in order not to test all the cases by hand every time but right now I do not have time to work on that so I will propose to open an issue so we can address into the future when I find some time if that is allright.

if we leave it for an issue, it can stay there for a very long time, especially if no one have an incline for unit testing. Maybe someone could help you with that so we do not defer that?

mardim91 commented 3 months ago

I am wondering if we could create some unit tests which could prove that this approach work and free of race conditions. Go tests can be run with -race flag to catch any race conditions: https://go.dev/doc/articles/race_detector

I have tested this code locally in order to check if the different corner cases that are resolved by this code here are working properly. Everything was working as it should be. The unit tests would be a very usefull tool to add in order not to test all the cases by hand every time but right now I do not have time to work on that so I will propose to open an issue so we can address into the future when I find some time if that is allright.

if we leave it for an issue, it can stay there for a very long time, especially if no one have an incline for unit testing. Maybe someone could help you with that so we do not defer that?

ok let me see what I can do about this

mardim91 commented 3 months ago

I am wondering if we could create some unit tests which could prove that this approach work and free of race conditions. Go tests can be run with -race flag to catch any race conditions: https://go.dev/doc/articles/race_detector

I have tested this code locally in order to check if the different corner cases that are resolved by this code here are working properly. Everything was working as it should be. The unit tests would be a very usefull tool to add in order not to test all the cases by hand every time but right now I do not have time to work on that so I will propose to open an issue so we can address into the future when I find some time if that is allright.

if we leave it for an issue, it can stay there for a very long time, especially if no one have an incline for unit testing. Maybe someone could help you with that so we do not defer that?

ok let me see what I can do about this

Hello @artek-koltun

I have one question regarding race detection. From what I understand from the link that you have provided I should not create any unit tests for catching race conditions but the only thing that I should do is to create some automatic github jobs that will build/run the programm with the --race parameter on (go build --race/ go run --race) is that correct or ? Can you please share your opinion on this ?

artek-koltun commented 2 months ago

I am wondering if we could create some unit tests which could prove that this approach work and free of race conditions. Go tests can be run with -race flag to catch any race conditions: https://go.dev/doc/articles/race_detector

I have tested this code locally in order to check if the different corner cases that are resolved by this code here are working properly. Everything was working as it should be. The unit tests would be a very usefull tool to add in order not to test all the cases by hand every time but right now I do not have time to work on that so I will propose to open an issue so we can address into the future when I find some time if that is allright.

if we leave it for an issue, it can stay there for a very long time, especially if no one have an incline for unit testing. Maybe someone could help you with that so we do not defer that?

ok let me see what I can do about this

Hello @artek-koltun

I have one question regarding race detection. From what I understand from the link that you have provided I should not create any unit tests for catching race conditions but the only thing that I should do is to create some automatic github jobs that will build/run the programm with the --race parameter on (go build --race/ go run --race) is that correct or ? Can you please share your opinion on this ?

We already run our uts with that flag https://github.com/opiproject/actions/blob/ae6a52f2fcf782b0c29df17e3c61475096ebb5d2/.github/workflows/go.yml#L35-L37

However, take in mind that it is not a compile time tool. It is a runtime tool. We need to execute a chunk of code, which can have race issues. Thus, we need a dedicated unit test which covers the code with the issue.

mardim91 commented 2 months ago

I am wondering if we could create some unit tests which could prove that this approach work and free of race conditions. Go tests can be run with -race flag to catch any race conditions: https://go.dev/doc/articles/race_detector

I have tested this code locally in order to check if the different corner cases that are resolved by this code here are working properly. Everything was working as it should be. The unit tests would be a very usefull tool to add in order not to test all the cases by hand every time but right now I do not have time to work on that so I will propose to open an issue so we can address into the future when I find some time if that is allright.

if we leave it for an issue, it can stay there for a very long time, especially if no one have an incline for unit testing. Maybe someone could help you with that so we do not defer that?

ok let me see what I can do about this

Hello @artek-koltun I have one question regarding race detection. From what I understand from the link that you have provided I should not create any unit tests for catching race conditions but the only thing that I should do is to create some automatic github jobs that will build/run the programm with the --race parameter on (go build --race/ go run --race) is that correct or ? Can you please share your opinion on this ?

We already run our uts with that flag https://github.com/opiproject/actions/blob/ae6a52f2fcf782b0c29df17e3c61475096ebb5d2/.github/workflows/go.yml#L35-L37

However, take in mind that it is not a compile time tool. It is a runtime tool. We need to execute a chunk of code, which can have race issues. Thus, we need a dedicated unit test which covers the code with the issue.

Hello @artek-koltun,

ok so from what I understand the unit tests are independent of the --race condition. The --race condition checks for races in specific parts of the code which code the unit tests are checking. Right ?

Also we have an agreeement to create unit tests no worries. We will create unit tests for the units (functions) that we are touching in the PR if that is allright

artek-koltun commented 2 months ago

Hello @artek-koltun,

ok so from what I understand the unit tests are independent of the --race condition. The --race condition checks for races in specific parts of the code which code the unit tests are checking. Right ?

Also we have an agreeement to create unit tests no worries. We will create unit tests for the units (functions) that we are touching in the PR if that is allright

Hej, You can use the following algorithm to write your unit test:

  1. Write a test which would fail with -race flag without a fix. (you might need to run multiple components/goroutines to create an env provoking the error)
  2. Write a fix
  3. Commit and push.
mardim91 commented 2 months ago

Hello @artek-koltun, ok so from what I understand the unit tests are independent of the --race condition. The --race condition checks for races in specific parts of the code which code the unit tests are checking. Right ? Also we have an agreeement to create unit tests no worries. We will create unit tests for the units (functions) that we are touching in the PR if that is allright

Hej, You can use the following algorithm to write your unit test:

  1. Write a test which would fail with -race flag without a fix. (you might need to run multiple components/goroutines to create an env provoking the error)
  2. Write a fix
  3. Commit and push.

Hello @artek-koltun ,

What do you mean write a test that would fail with -race flag. I do not think that we have any race problems in the code today so we can catch them by unit test and fix them afterwards.

What we are planning to so is that we will write unit tests for the code changes in this commit, we will run the tests with -race flag on and if in the meanwhile find any race problem then we will push a fix.

I hope we are in the same page.

artek-koltun commented 2 months ago

Hello @artek-koltun, ok so from what I understand the unit tests are independent of the --race condition. The --race condition checks for races in specific parts of the code which code the unit tests are checking. Right ? Also we have an agreeement to create unit tests no worries. We will create unit tests for the units (functions) that we are touching in the PR if that is allright

Hej, You can use the following algorithm to write your unit test:

  1. Write a test which would fail with -race flag without a fix. (you might need to run multiple components/goroutines to create an env provoking the error)
  2. Write a fix
  3. Commit and push.

Hello @artek-koltun ,

What do you mean write a test that would fail with -race flag. I do not think that we have any race problems in the code today so we can catch them by unit test and fix them afterwards.

What we are planning to so is that we will write unit tests for the code changes in this commit, we will run the tests with -race flag on and if in the meanwhile find any race problem then we will push a fix.

I hope we are in the same page.

I might have lost the context. Pls create ut to the behavior you are fixing.