Admiral-Piett / goaws

AWS (SQS/SNS) Clone for Development testing
MIT License
767 stars 144 forks source link

JSON API - sqs.ReceiveMessage, sqs.ChangeMessageVisibility, sqs.DeleteMessage #301

Closed andrew-womeldorf closed 1 month ago

andrew-womeldorf commented 1 month ago
andrew-womeldorf commented 1 month ago

I know there's a lot of cross-over in these, but in future, if you don't mind, doing 1 endpoint per PR would help me review it and track things long term. I'd really appreciate it.

Yes, sorry about that :)

Also, at the end of all this, would you mind squashing your commits?

Squashed!

andrew-womeldorf commented 1 month ago

Ok, I've cleaned up those tests. Thanks for pointing me back there - I had copy/pasted the old tests and hadn't bothered to look at them much. I think it's good to have cleaned them up a bit.

There's a couple of requests in the tests which I left as form requests because the actions couldn't be mocked by interacting directly with the queue, or a request needed to occur to properly reproduce the expected functionality and the action hasn't yet been migrated to the new models.

And re-squashed.

Admiral-Piett commented 1 month ago

Ah, sorry - probably need a rebase after Koji's change?

andrew-womeldorf commented 1 month ago

Rebased, Resquashed

Admiral-Piett commented 1 month ago

@andrew-womeldorf Looks like the unit tests failed - something about a race condition in the memory I think during TestReceiveMessage_CanceledByClientV1.

==================
WARNING: DATA RACE
Write at 0x00c000348108 by goroutine 69:
  github.com/Admiral-Piett/goaws/app/gosqs.TestReceiveMessage_CanceledByClientV1()
      /home/runner/work/goaws/goaws/app/gosqs/receive_message_test.go:101 +0x51d
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1486 +0x47

Previous read at 0x00c00034[81](https://github.com/Admiral-Piett/goaws/actions/runs/9207849030/job/25342128834?pr=301#step:5:82)08 by goroutine 70:
  github.com/Admiral-Piett/goaws/app/gosqs.ReceiveMessageV1()
      /home/runner/work/goaws/goaws/app/gosqs/receive_message.go:62 +0x684
  github.com/Admiral-Piett/goaws/app/gosqs.TestReceiveMessage_CanceledByClientV1.func2()
      /home/runner/work/goaws/goaws/app/gosqs/receive_message_test.go:89 +0x4b2

Goroutine 69 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1486 +0x724
  testing.runTests.func1()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1[83](https://github.com/Admiral-Piett/goaws/actions/runs/9207849030/job/25342128834?pr=301#step:5:84)9 +0x99
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1439 +0x213
  testing.runTests()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1837 +0x7e4
  testing.(*M).Run()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1719 +0xa71
  github.com/Admiral-Piett/goaws/app/gosqs.TestMain()
      /home/runner/work/goaws/goaws/app/gosqs/gosqs_test.go:20 +0x1fa
  main.main()
      _testmain.go:221 +0x3f7

Goroutine 70 (finished) created at:
  github.com/Admiral-Piett/goaws/app/gosqs.TestReceiveMessage_CanceledByClientV1()
      /home/runner/work/goaws/goaws/app/gosqs/receive_message_test.go:81 +0x39d
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:14[86](https://github.com/Admiral-Piett/goaws/actions/runs/9207849030/job/25342128834?pr=301#step:5:87) +0x47
==================
--- FAIL: TestReceiveMessage_CanceledByClientV1 (0.11s)
Error:     testing.go:1312: race detected during execution of test
andrew-womeldorf commented 1 month ago

Ah, yep. That makes sense.

I've pushed a fix, and tested locally with the -race flag this time. I was unfamiliar with that before! TIL!

Admiral-Piett commented 1 month ago

@andrew-womeldorf Awesome! Any reason why we want to close this? We were close, I thought. If we can get the tests to pass, I'll merge it?

andrew-womeldorf commented 1 month ago

I closed it? That was an accident! Sorry! I'll reopen