Conversation
|
@rmothilal : The unit test check is failing because it now needs to run with jest, not tape + tape-xunit. I can make junit style output happen (via |
Hi @jgeewax, we currently have implementations of jest testing that use jest-junit and have the appropriate configuration in the circleci config. I also noticed that you have test config out of the test folder i would recommend creating a base folder in test and storing all appropriate config in there. Please see examples of jest testing in the following repos: Please follow a similar pattern and use similar dependencies where possible. |
|
That's fantastic -- thanks so much for the pointer! I'll go try to make this repo look more like those. |
208f44a to
5ac16d0
Compare
|
@rmothilal : OK -- all updated to look like the others. Let me know if I missed anything? |
| @@ -0,0 +1,153 @@ | |||
| /***** | |||
There was a problem hiding this comment.
I think we do not need this producer. As far as I remember this service only consumes so we can remove it. Sorry for leaving the producer.js- there :(
There was a problem hiding this comment.
OK - I can remove this if you're sure it's not worth keeping 👍
There was a problem hiding this comment.
@jgeewax i do not understand the massive rewrite of code for jest testing.
Please see comments, there are more to come, just didnt want to put too many in one review.
I also think that once you bring in the central-services-stream library it should greatly reduce the size of the PR
| /***** | ||
| License | ||
| -------------- | ||
| Copyright © 2017 Bill & Melinda Gates Foundation | ||
| The Mojaloop files are made available by the Bill & Melinda Gates Foundation | ||
| under the Apache License, Version 2.0 (the "License") and you may not use | ||
| these files except in compliance with the License. You may obtain a copy of | ||
| the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, the Mojaloop files | ||
| are distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| KIND, either express or implied. See the License for the specific language | ||
| governing permissions and limitations under the License. | ||
|
|
||
| Contributors | ||
| -------------- | ||
| This is the official list of the Mojaloop project contributors for this file. | ||
| Names of the original copyright holders (individuals or organizations) | ||
| should be listed with a '*' in the first column. People who have | ||
| contributed from an organization can be listed under the organization | ||
| that actually holds the copyright for their contributions (see the | ||
| Gates Foundation organization for an example). Those individuals should have | ||
| their names indented and be marked with a '-'. Email address can be added | ||
| optionally within square brackets <email>. | ||
|
|
||
| * Gates Foundation | ||
| - Name Surname <name.surname@gatesfoundation.com> | ||
|
|
||
| * Rajiv Mothilal <rajiv.mothilal@modusbox.com> | ||
| * Miguel de Barros <miguel.debarros@modusbox.com> | ||
| * JJ Geewax <jjg@google.com> | ||
|
|
||
| -------------- | ||
| ******/ | ||
| 'use strict' | ||
|
|
||
| /** | ||
| * @module src/lib/kafka/producer | ||
| */ | ||
|
|
||
| const Producer = require('@mojaloop/central-services-stream').Kafka.Producer | ||
| const Logger = require('@mojaloop/central-services-shared').Logger | ||
|
|
||
| let _listOfProducers = {} | ||
|
|
||
| class _Producer { | ||
| static get listOfProducers () { | ||
| return _listOfProducers | ||
| } | ||
|
|
||
| static set listOfProducers (value) { | ||
| _listOfProducers = {} | ||
| } | ||
|
|
||
| /** | ||
| * @function produceMessage | ||
| * | ||
| * @param {string} messageProtocol - message being created against topic | ||
| * @param {object} topicConf - configuration for the topic to produce to | ||
| * @param {object} config - Producer configuration, eg: to produce batch or | ||
| * poll | ||
| * | ||
| * @description Creates a producer on Kafka for the specified topic and | ||
| * configuration | ||
| * | ||
| * @returns {boolean} - returns true if producer successfully created and | ||
| * producers to | ||
| * @throws {error} - if not successfully create/produced to | ||
| */ | ||
| static async produceMessage (messageProtocol, topicConf, config) { | ||
| try { | ||
| let producer | ||
| if (this.listOfProducers[topicConf.topicName]) { | ||
| producer = this.listOfProducers[topicConf.topicName] | ||
| } else { | ||
| Logger.info('Producer::start::topic=' + topicConf.topicName) | ||
| producer = new Producer(config) | ||
| Logger.info('Producer::connect::start') | ||
| await producer.connect() | ||
| Logger.info('Producer::connect::end') | ||
| this.listOfProducers[topicConf.topicName] = producer | ||
| } | ||
| Logger.info(`Producer.sendMessage:: messageProtocol:' | ||
| ${JSON.stringify(messageProtocol)}'`) | ||
| await producer.sendMessage(messageProtocol, topicConf) | ||
| Logger.info('Producer::end') | ||
| return true | ||
| } catch (e) { | ||
| Logger.error(e) | ||
| Logger.info('Producer error has occurred') | ||
| throw e | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @function disconnect | ||
| * | ||
| * @param {string} topicName - Producer of the specified topic to be | ||
| * disconnected. If this is null, then ALL | ||
| * producers will be disconnected. | ||
| * Defaults: null. | ||
| * | ||
| * @description Disconnects a specific producer, or ALL producers from Kafka | ||
| * | ||
| * @returns {object} Promise | ||
| */ | ||
| static async disconnect (topicName = null) { | ||
| if (topicName && typeof topicName === 'string') { | ||
| await this.getProducer(topicName).disconnect() | ||
| } else if (topicName === null) { | ||
| let isError = false | ||
| const errorTopicList = [] | ||
|
|
||
| let tpName | ||
| for (tpName in this.listOfProducers) { | ||
| try { | ||
| await this.getProducer(tpName).disconnect() | ||
| } catch (e) { | ||
| isError = true | ||
| errorTopicList.push({ topic: tpName, error: e.toString() }) | ||
| } | ||
| } | ||
| if (isError) { | ||
| throw Error('The following Producers could not be disconnected: ' + | ||
| JSON.stringify(errorTopicList)) | ||
| } | ||
| } else { | ||
| throw Error(`Unable to disconnect Producer: ${topicName}`) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @function getProducer | ||
| * | ||
| * @param {string} topicName - the topic name to locate a specific producer | ||
| * | ||
| * @description This is used to get a producer with the topic name to send | ||
| * messages to a Kafka topic | ||
| * | ||
| * @returns {Producer} - Returns consumer | ||
| * @throws {Error} - if consumer not found for topic name | ||
| */ | ||
| static getProducer (topicName) { | ||
| if (this.listOfProducers[topicName]) { | ||
| return this.listOfProducers[topicName] | ||
| } else { | ||
| throw Error(`No producer found for topic ${topicName}`) | ||
| } | ||
| } | ||
| } | ||
| module.exports = _Producer |
There was a problem hiding this comment.
this functionality is already in the central-services-stream library.
Please use that
There was a problem hiding this comment.
We do not produce in that module, so this file can be removed. Sorry for me haven't done that. :/
| /** | ||
| * @function isConnected | ||
| * | ||
| * @param {string} topicName - the topic name of the consumer to check | ||
| * | ||
| * @description Use this to determine whether or not we are connected to the | ||
| * broker. Internally, it calls `getMetadata` to determine | ||
| * if the broker client is connected. | ||
| * | ||
| * @returns {true} - if connected | ||
| * @throws {Error} - if consumer can't be found or the consumer isn't connected | ||
| */ | ||
| static async isConnected (topicName) { | ||
| // This "module.exports" thing is important for mocking in the tests. | ||
| // TODO: Rearrange this file so that we don't need to do this. | ||
| const consumer = this.getConsumer(topicName) | ||
| const getMetadata = util.promisify(consumer.getMetadata) | ||
| const metadata = await getMetadata({ | ||
| topicName, | ||
| timeout: 3000 | ||
| }) | ||
| const foundTopics = metadata.topics.map(topic => topic.name) | ||
| if (foundTopics.indexOf(topicName) === -1) { | ||
| Logger.debug(`Connected to consumer, but ${topicName} not found.`) | ||
| throw ErrorHandler.Factory.createInternalServerFSPIOPError( | ||
| `Connected to consumer, but ${topicName} not found.`) | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
this functionality is in central-services-stream library please use that
| autoCommitEnabled, | ||
| connectedTimeStamp | ||
| }) | ||
| }) |
There was a problem hiding this comment.
this functionality is catered for in the central-services-stream library. please use that functionality
| /** | ||
| * @function getConsumerMetadata | ||
| * | ||
| * @param {string} topicName - the topic name to locate a specific consumer | ||
| * | ||
| * @description This is used to retrieve a consumer for a given topic name | ||
| * | ||
| * @returns {Consumer} - Returns the registered consumer for a given topic name | ||
| * @throws {Error} - if no consumer is found for a topic name | ||
| */ | ||
| static getConsumerMetadata (topicName) { | ||
| const metadata = this._topicConsumerMetadata | ||
| if (topicName in metadata) { | ||
| return metadata[topicName] | ||
| } else { | ||
| // If the topicName doesn't exist in the map, throw an error. | ||
| throw ErrorHandler.Factory.createInternalServerFSPIOPError( | ||
| `No consumer found for topic ${topicName}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
this is catered for in the central-services-stream library, please use that fucntionality
| */ | ||
| static getConsumer (topicName) { | ||
| // This is just here for legacy reasons | ||
| return this.getKafkaConsumer(topicName) |
There was a problem hiding this comment.
this functionality is catered for by the central-services-stream library, please use that functionality
There was a problem hiding this comment.
you can see here how to set the consumer with the latest libraries https://github.com/mojaloop/event-stream-processor/blob/master/src/setup.js
| "test:coverage-check": "jest --coverage --testMatch '**/test/unit/**/*.test.js'", | ||
| "test:junit": "jest --reporters=default --reporters=jest-junit --testMatch '**/test/unit/**/*.test.js'", | ||
| "audit:resolve": "SHELL=sh resolve-audit && sed -i 's/: \"^/: \"/' package.json", | ||
| "audit:check": "SHELL=sh check-audit" |
There was a problem hiding this comment.
please add the following below audit:check
"dep:check": "npx ncu -e 2", "dep:update": "npx ncu -u"
| "test:junit": "jest --reporters=default --reporters=jest-junit --testMatch '**/test/unit/**/*.test.js'", | ||
| "audit:resolve": "SHELL=sh resolve-audit && sed -i 's/: \"^/: \"/' package.json", | ||
| "audit:check": "SHELL=sh check-audit" | ||
| }, |
There was a problem hiding this comment.
please add the following after the scripts object.
"pre-commit": [ "standard", "dep:check", "test" ],
| "nodemailer-mock": "1.4.3", | ||
| "nodemon": "1.19.1", | ||
| "npm-audit-resolver": "1.5.0", | ||
| "npm-run-all": "4.1.5", |
There was a problem hiding this comment.
please install the dev dependency
npm-check-updates
| * @returns {Array<string>} - list of topics | ||
| */ | ||
| static getListOfTopics () { | ||
| return Object.keys(this._topicConsumerMetadata) |
There was a problem hiding this comment.
this functionality is catered for by the central-services-stream library, please use that functionality
vgenev
left a comment
vgenev
left a comment
There was a problem hiding this comment.
Just out of curiosity why made everything Classes :)
|
@rmothilal : I just wanted to clarify that my goal was to move to Jest without changing any code except tests, however I ran into a couple of issues with that:
So this leads me to the obvious question, which of the following options sounds best to you:
I suspect Option 2 will be the quickest (and probably safest, given the new test coverage that gets added in this PR), however the other options are totally doable as well, so I just wanted to confirm with you before getting back to work. |
Hey @jgeewax I would believe that switching to central-services first and then doing the testing for jest would result in much less code being created and saving of lots of time. So option 3 would be my guess |
|
OK - cool. Let's put this one on hold then. I'll start going through what central services has on hand, port this to use that while adding more tape tests for that, and then move to jest once we have that covered. |
|
@jgeewax I have located the PR, maybe if you can pull in changes from master, update your branch and then i can review and approve |
|
Accidentally closed. |
|
|
5 participants
I'm sorry in advance that this is such a big nasty scary pull request. I only changed actual code in cases where mocking wasn't possible to do safely (and in those cases, added extra tests for the new code).
I couldn't find a way to do a piecemeal Jest migration (since the tests either needed to be fully on Jest or fully in Tape), but if there is a better way, let me know.