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

[BE2] Fix be2 test db ref injection #1892

Merged
merged 10 commits into from
Jun 3, 2024

Conversation

DanielTavaresA
Copy link
Collaborator

fixes the issue of using the wrong refs for the database.

Now all refs from code come from the PublishSubscribe graph, and if something should be injected, it is being done through PublishSubscribe.buildgraph(...)

Copy link

Pull reviewers stats

Stats of the last 30 days for popstellar:

User Total reviews Time to review Total comments
K1li4nL
🥇
10
▀▀
4d 21h 51m
23
pierluca
🥈
5
6d 22h 1m
8
onsriahi14
🥉
5
3d 1h 48m
10
matteosz
5
4d 1h 36m
22
simone-kalbermatter
4
1d 14h 46m
0
quadcopterman
4
5d 19h 9m
1
arnauds5
3
8d 13h 44m
8
Tyratox
2
27d 19h 54m
▀▀▀
48
▀▀▀
t1b00
2
6d 6h 52m
4
osm-alt
2
6d 22h 53m
12
ljankoschek
2
18m
0
DanielTavaresA
2
2d 10h 22m
8
Kaz-ookid
2
7d 15h 16m
7
1florentin
1
8d 13h 49m
6
sgueissa
1
6d 11h 44m
1
emonnin-epfl
1
51m
1
⚡️ Pull request stats

@DanielTavaresA DanielTavaresA marked this pull request as ready for review May 29, 2024 16:12
@DanielTavaresA DanielTavaresA requested a review from a team as a code owner May 29, 2024 16:12
Copy link
Contributor

@K1li4nL K1li4nL left a comment

Choose a reason for hiding this comment

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

I am not entirely sure what these changes means for the overall design, it reduces a level of indirection in the handlers which is nice but means PublishSubscribe is a hard dependency for the entire system. I don't see any other way to fix the problem you are trying to fix without a larger changes right now so we might as well go that way.

@@ -70,6 +70,7 @@ trait MessageHandler extends AskPatternConstants {
def dbAskWritePropagate(rpcRequest: JsonRpcRequest): Future[GraphMessage] = {
rpcRequest.getParamsMessage match
case Some(m) =>
println(s"dbActorWriteAndPropagate : ${dbActor.actorRef}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is might log a bit too much

@@ -214,142 +218,144 @@ class ElectionHandlerTest extends TestKit(ActorSystem("Election-DB-System")) wit
system.actorOf(dbActorMock)
}

private def injectDb(dbRef: AskableActorRef) = PublishSubscribe.buildGraph(Actor.noSender, dbRef, Actor.noSender, MessageRegistry(), Actor.noSender, Actor.noSender, Actor.noSender, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in this case not, because different types of mocked db are injected. Using BeforeAndAfterEach would force to inject the same db for all test.

Copy link

sonarcloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed for 'PoP - PoPCHA-Web-Client'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Jun 3, 2024

Copy link

sonarcloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Be1-Go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@DanielTavaresA
Copy link
Collaborator Author

I am not entirely sure what these changes means for the overall design, it reduces a level of indirection in the handlers which is nice but means PublishSubscribe is a hard dependency for the entire system. I don't see any other way to fix the problem you are trying to fix without a larger changes right now so we might as well go that way.

Agree, buildGraph is tight to adding references to the system, which doesn't seem a natural behaviour. Overall, a lot of references are passed around, eventhought they are unique in the system. We could try to centralize those references using maybe https://doc.akka.io/docs/akka/current/typed/actor-discovery.html as you recommended.

Copy link

sonarcloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Fe1-Web'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Fe2-Android'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

@K1li4nL K1li4nL left a comment

Choose a reason for hiding this comment

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

LGTM

@K1li4nL K1li4nL added this pull request to the merge queue Jun 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 3, 2024
@DanielTavaresA DanielTavaresA added this pull request to the merge queue Jun 3, 2024
Merged via the queue into master with commit 637103f Jun 3, 2024
18 checks passed
@DanielTavaresA DanielTavaresA deleted the fix-be2-daniel-test-db-ref-injection branch June 3, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ActorRef injection through PublishSubscribe creates inconsistencies in tests
2 participants