Skip to content

Conversation

@maxceem
Copy link
Contributor

@maxceem maxceem commented Aug 16, 2019

Implementation for #357

Creating 2 topics for works to show comments on Details and Requirements tabs.

Notes:

  • Keep the same code base between phases and works, so later we can update the common functionality in the same place
  • Implemented unit tests, to verify that for phases we create/update/delete one topic as before. And that for new work endpoints we create/update/delete two topics.

Tested it locally and also have unit tests. So I think it's safe to merge and continue testing on DEV.

@maxceem maxceem requested a review from vikasrohit August 16, 2019 10:09
@vikasrohit
Copy link

@maxceem It seems something is changed in terms of unit tests setup, 138 unit tests are failing on CircleCI.

@maxceem
Copy link
Contributor Author

maxceem commented Aug 16, 2019

@vikasrohit yep, checking...

@maxceem
Copy link
Contributor Author

maxceem commented Aug 19, 2019

@vikasrohit Unit tests for this PR are fixed now. The main issue was that CircleCI config didn't have RabbitMQ set up. So I've updated .circleci/config.yml.

I think this PR can merged now.

@vikasrohit
Copy link

Thanks @maxceem However, I am little confused how the build was working fine earlier then?

@maxceem
Copy link
Contributor Author

maxceem commented Aug 19, 2019

The difference between before and now is that before we only mock RabbitMQ. But for new tests regarding creating topics for phases and works, we had to un-mock RabbitMQ for these tests, i. e. we have to use real RabbitMQ setup, as topics are created by listening to RabbitMQ events.

@vikasrohit
Copy link

Thanks for the info @maxceem Now when we have the rabbitmq working in the CI/CD, we can add unit tests for more event based processing in this service. Could you please log an issue for that, we would take care of that later.

@maxceem
Copy link
Contributor Author

maxceem commented Aug 19, 2019

Issue created #371

@vikasrohit
Copy link

@maxceem one test is failing after merging it to dev.

 1) projectUpdatedKafkaHandler integration "before each" hook for "should throw exception when project not found by id":
     [mapper_parsing_exception] failed to parse [doc.lastActivityAt]
  :: {"path":"/projects_test/projectV4/1","query":{},"body":"{\"doc\":{\"createdAt\":\"2019-08-19T10:14:13.000Z\",\"updatedAt\":\"2019-08-19T10:14:13.000Z\",\"terms\":[],\"version\":\"v3\",\"id\":1,\"type\":\"generic\",\"billingAccountId\":1,\"name\":\"test1\",\"description\":\"test project1\",\"status\":\"draft\",\"details\":{},\"createdBy\":1,\"updatedBy\":1,\"lastActivityAt\":\"1970-01-01T00:00:00.001Z\",\"lastActivityUserId\":\"1\",\"directProjectId\":null,\"external\":null,\"bookmarks\":null,\"utm\":null,\"estimatedPrice\":null,\"actualPrice\":null,\"challengeEligibility\":null,\"cancelReason\":null,\"templateId\":null,\"deletedAt\":null,\"deletedBy\":null}}","statusCode":400,"response":"{\"error\":{\"root_cause\":[{\"type\":\"mapper_parsing_exception\",\"reason\":\"failed to parse [doc.lastActivityAt]\"}],\"type\":\"mapper_parsing_exception\",\"reason\":\"failed to parse [doc.lastActivityAt]\",\"caused_by\":{\"type\":\"number_format_exception\",\"reason\":\"For input string: \\\"1970-01-01T00:00:00.001Z\\\"\"}},\"status\":400}"}
      at respond (node_modules/elasticsearch/src/lib/transport.js:289:15)
      at checkRespForFailure (node_modules/elasticsearch/src/lib/transport.js:248:7)
      at HttpConnector.<anonymous> (node_modules/elasticsearch/src/lib/connectors/http.js:164:7)
      at IncomingMessage.wrapper (node_modules/elasticsearch/node_modules/lodash/index.js:3095:19)
      at endReadableNT (_stream_readable.js:1055:12)
      at node_modules/async-listener/glue.js:188:31
      at _combinedTickCallback (internal/process/next_tick.js:138:11)
      at process._tickCallback (internal/process/next_tick.js:180:9)

@maxceem
Copy link
Contributor Author

maxceem commented Aug 19, 2019

@vikasrohit works on the third attempt:

image

@vikasrohit
Copy link

It failed again. :(

@maxceem
Copy link
Contributor Author

maxceem commented Aug 21, 2019

The last fail was strange as it looks like there is RabbitMQ in the docker, it shouldn't happen as I've already configured the CircleCI for RabbitMQ. I re-run again and it works.

@vikasrohit
Copy link

May be we need to add some time delay to wait for RabbitMQ to start properly before actual tests start.

@vikasrohit
Copy link

I tried to rerun it and it failed again. So, I think there is something definitely wrong here. And the error is consistent:
failed to parse [doc.lastActivityAt]\",\"caused_by\":{\"type\":\"number_format_exception\",\"reason\":\"For input string: \\\"1970-01-01T00:00:00.001Z\\\"\"

@maxceem
Copy link
Contributor Author

maxceem commented Aug 21, 2019

Yeah, this error is not because of CircleCI setup. Cannot reproduce it locally even once, but will try to search for possible reasons.

@vikasrohit
Copy link

May be this error happens when we rebuild the docker images (used in local setup) because on circle ci every time it fetches the docker images so essentially it starts from scratch on CircleCI while on our local setup, we have the basic setup already working for every test case run. It caused issues in past as well when we were asserting the ids of the new records in unit tests the reason was CircleCI has primary key sequences reset on every run while our local setup does not.

@maxceem
Copy link
Contributor Author

maxceem commented Aug 21, 2019

Hmm, so far there two types of failing build.

  1. The error which happened before with multiple tests failed could be caused by CircleCI, as actually it shouldn't happen after I've added RabbitMQ to circle config.

  2. The error from the last failed build mentioned in this comment most likely is caused by tests interfering with each other. Will check it out.

If the first case with multiple errors would happen again, would have to check it out also.

@vikasrohit
Copy link

  1. the link to the build seems wrong @maxceem as there is only 1 test failing in that build. I think you meant this one. If you are concerned about this one, I think we are good with that as now we have the rabbitmq in circle ci.
  2. Yep, this is one we have to take care of.

@eisbilir eisbilir deleted the feature/topics-for-works branch November 9, 2023 20:02
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.

4 participants