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

Update MemoryQueueClient::getMessages method #34

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

bartoszwidera
Copy link
Contributor

For more complexity of testing the messages should be available to return and still exist in memory.

@bartoszwidera bartoszwidera self-assigned this Feb 29, 2024
@bartoszwidera
Copy link
Contributor Author

Testowanie MemoryQueueClient jest ciężkie bo nie można zrobić dwóch assercji na każdą wysłaną wiadomość, zważywszy, że pobieranie wiadomości jest modyfikowane przez array_splice.

@bartoszwidera
Copy link
Contributor Author

Release już będzie z 1.03 -> 1.1.0 (Doszła nowa funkcjonalność).

@devofdisaster
Copy link
Member

Nie prościej po prostu zrobić public array $memory = []; w MemoryQueueClient? :P
Pobieranie wszystkich message'y nie ma sensu w implementacji SQS.

W ogóle nie bardzo rozumiem jaki jest case że to jest potrzebne, nie da się wywołać getMessages() i robić asercje na tym arrayu?

@bartoszwidera bartoszwidera changed the title Add new QueueClient::getAllMessages() method Make MemoryQueueClient::$memory as public property Feb 29, 2024
@bartoszwidera
Copy link
Contributor Author

Też można :jeszczejak:

W ogóle nie bardzo rozumiem jaki jest case że to jest potrzebne, nie da się wywołać getMessages() i robić asercje na tym arrayu?

Chciałbym zrobić test, który sprawdza dwie wysłane wiadomości aka dwie asercje. Wyciągnięcie dwa razy wiadomości z MemoryQueueClient spowoduje, że za drugim razem już wiadomości mieć nie będę #array_splice.

@bartoszwidera
Copy link
Contributor Author

@devofdisaster @nexik @maureis jest git?

@nexik
Copy link

nexik commented Mar 4, 2024

ja bym zostawil (dawanie public prywtne property aby naprawic buga kodzie) to a zamiast tego poprawil buga w getMessages()

wedlug mnie to bug bo zakladam ze autor mial na mysli array_slice a nie array_splice poniewaz splice ma na celu modyfikwacje (podmiane) tablicy a nie wyciaganie tylko czesci tablicy. w tym kodzie $replacement z array_splice jest pominiety, co wskazuje na duze prawdopodobienstwo wdrozenie buga przez blad osoby

POniewaz pierwsze parametry sa takie same to usuniemy tylko p i zamist array_splice bedzie array_slice
Pozwoli to na odpalenie w testach getMessages() wielokrotnie

Dodatkowo mysle ze splice to pomylka bo getMessages() nie powinno usuwac wiadomosci z kolejki. Do tego powinna byc osobna funkcja removeMessage() natomiast aws-bundle jest gluwnie uzywana tylko do wysylki wiadomosci bo w projekcie queue mamy osobna implementacje.

@bartoszwidera bartoszwidera changed the title Make MemoryQueueClient::$memory as public property Update MemoryQueueClient::getMessages method Mar 4, 2024
@bartoszwidera
Copy link
Contributor Author

Fakt mogła być to pomyłka (zważywszy, parametry są praktycznie takie same).

Fixnięte 🔨

Copy link

@nexik nexik left a comment

Choose a reason for hiding this comment

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

🚤 LGTM

@bartoszwidera bartoszwidera merged commit 6453499 into master Mar 4, 2024
2 checks passed
@bartoszwidera bartoszwidera deleted the upgrade-queue-client branch March 4, 2024 06:40
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.

3 participants