Updated email-notifier to use Jest for testing. #75

jgeewax · 2019-11-13T07:21:13Z

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.

jgeewax · 2019-11-13T08:13:39Z

@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 jest-junit), but we might want to change where the output goes and whatnot on the Circle CI configuration... How do you suggest we handle this one?

rmothilal · 2019-11-13T09:00:11Z

@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 jest-junit), but we might want to change where the output goes and whatnot on the Circle CI configuration... How do you suggest we handle this one?

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:
https://github.com/mojaloop/account-lookup-service
https://github.com/mojaloop/transaction-requests-service

Please follow a similar pattern and use similar dependencies where possible.
Let me know if you require further information or assistance

jgeewax · 2019-11-13T09:28:50Z

That's fantastic -- thanks so much for the pointer! I'll go try to make this repo look more like those.

jgeewax · 2019-11-14T06:36:47Z

@rmothilal : OK -- all updated to look like the others. Let me know if I missed anything?

vgenev · 2019-11-14T13:02:03Z

src/lib/kafka/producer.js

@@ -0,0 +1,153 @@
+/*****


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 :(

OK - I can remove this if you're sure it's not worth keeping 👍

rmothilal

@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

rmothilal · 2019-11-14T12:41:19Z

src/lib/kafka/producer.js

+    _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


this functionality is already in the central-services-stream library.
Please use that

We do not produce in that module, so this file can be removed. Sorry for me haven't done that. :/

rmothilal · 2019-11-14T12:43:39Z

src/lib/kafka/consumer.js

+  /**
+   * @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
+  }


this functionality is in central-services-stream library please use that

rmothilal · 2019-11-14T12:45:32Z

src/lib/kafka/consumer.js

+        autoCommitEnabled,
+        connectedTimeStamp
+      })
+    })


this functionality is catered for in the central-services-stream library. please use that functionality

rmothilal · 2019-11-14T12:47:04Z

src/lib/kafka/consumer.js

+  /**
+   * @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}`)
+    }
+  }


this is catered for in the central-services-stream library, please use that fucntionality

rmothilal · 2019-11-14T12:48:01Z

src/lib/kafka/consumer.js

+   */
+  static getConsumer (topicName) {
+    // This is just here for legacy reasons
+    return this.getKafkaConsumer(topicName)


this functionality is catered for by the central-services-stream library, please use that functionality

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

rmothilal · 2019-11-14T12:49:52Z

package.json

+    "test:unit": "jest --testMatch '**/test/unit/**/*.test.js'",
+    "test:coverage": "jest --coverage --coverageThreshold='{}' --testMatch '**/test/unit/**/*.test.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"


please add the following below audit:check

"dep:check": "npx ncu -e 2", "dep:update": "npx ncu -u"

rmothilal · 2019-11-14T12:50:59Z

package.json

+    "test:unit": "jest --testMatch '**/test/unit/**/*.test.js'",
+    "test:coverage": "jest --coverage --coverageThreshold='{}' --testMatch '**/test/unit/**/*.test.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"
  },


please add the following after the scripts object.

"pre-commit": [ "standard", "dep:check", "test" ],

rmothilal · 2019-11-14T12:52:12Z

package.json

    "nodemailer-mock": "1.4.3",
    "nodemon": "1.19.1",
+    "npm-audit-resolver": "1.5.0",
    "npm-run-all": "4.1.5",


please install the dev dependency
npm-check-updates

rmothilal · 2019-11-14T12:53:13Z

src/lib/kafka/consumer.js

+   * @returns {Array<string>} - list of topics
+   */
+  static getListOfTopics () {
+    return Object.keys(this._topicConsumerMetadata)


this functionality is catered for by the central-services-stream library, please use that functionality

vgenev

Just out of curiosity why made everything Classes :)

Rajiv has a point for the consumer

jgeewax · 2019-11-15T02:29:08Z

@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:

The test coverage was not up to the requirement (90% is what we have generally). This meant that I needed to write additional tests for modules.
Some files were not tested at all, and others were tested but not actively run, so enabling them to run meant there was quite a lot of fixing to do.
Quite a few modules used rewire and proxyquire, which don't appear to be supported by Jest. As a result, I figured the best option would be to refactor these modules (mostly collections of functions with a single exported function) into static classes so that the exported method signature and functionality remained the same, but the tests were easier to write using sinon for stubbing and spying.

So this leads me to the obvious question, which of the following options sounds best to you:

Submit one big PR that switches us over to using central-services for common functionality as well as using Jest for testing.
Submit a PR that uses Jest for testing, then another PR that switches us to central-services.
Submit a PR that switches us to central-services, then another PR that uses Jest for testing.

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.

rmothilal · 2019-11-15T12:09:15Z

@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:

The test coverage was not up to the requirement (90% is what we have generally). This meant that I needed to write additional tests for modules.

Some files were not tested at all, and others were tested but not actively run, so enabling them to run meant there was quite a lot of fixing to do.

Quite a few modules used rewire and proxyquire, which don't appear to be supported by Jest. As a result, I figured the best option would be to refactor these modules (mostly collections of functions with a single exported function) into static classes so that the exported method signature and functionality remained the same, but the tests were easier to write using sinon for stubbing and spying.

So this leads me to the obvious question, which of the following options sounds best to you:

Submit one big PR that switches us over to using central-services for common functionality as well as using Jest for testing.

Submit a PR that uses Jest for testing, then another PR that switches us to central-services.

Submit a PR that switches us to central-services, then another PR that uses Jest for testing.

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

jgeewax · 2019-11-18T03:11:52Z

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.

rmothilal · 2020-02-11T16:33:22Z

@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

lewisdaly · 2020-02-19T03:34:21Z

Accidentally closed.

CLAassistant · 2020-10-25T11:24:52Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

jgeewax requested a review from rmothilal November 13, 2019 07:22

jgeewax force-pushed the jest branch 3 times, most recently from 208f44a to 5ac16d0 Compare November 14, 2019 06:26

Updated email-notifier to use Jest for testing.

6da0c0c

jgeewax force-pushed the jest branch from 5ac16d0 to 6da0c0c Compare November 14, 2019 06:30

rmothilal requested review from vgenev and mdebarros November 14, 2019 12:55

rmothilal assigned jgeewax Nov 14, 2019

vgenev reviewed Nov 14, 2019

View reviewed changes

rmothilal requested changes Nov 14, 2019

View reviewed changes

vgenev previously approved these changes Nov 14, 2019

View reviewed changes

rmothilal requested review from rmothilal and vgenev February 11, 2020 15:53

lewisdaly closed this Feb 19, 2020

lewisdaly reopened this Feb 19, 2020

snyk-bot mentioned this pull request Apr 7, 2022

[Snyk] Fix for 1 vulnerabilities rasputtintin/email-notifier#15

Open

rasputtintin mentioned this pull request May 19, 2022

[Snyk] Fix for 2 vulnerabilities rasputtintin/email-notifier#17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated email-notifier to use Jest for testing. #75

Updated email-notifier to use Jest for testing. #75

jgeewax commented Nov 13, 2019

jgeewax commented Nov 13, 2019

rmothilal commented Nov 13, 2019

jgeewax commented Nov 13, 2019

jgeewax commented Nov 14, 2019

vgenev Nov 14, 2019 •

edited

Loading

jgeewax Nov 15, 2019

rmothilal left a comment •

edited

Loading

rmothilal Nov 14, 2019

vgenev Nov 14, 2019

rmothilal Nov 14, 2019

rmothilal Nov 14, 2019

rmothilal Nov 14, 2019

rmothilal Nov 14, 2019

vgenev Nov 14, 2019 •

edited

Loading

rmothilal Nov 14, 2019

rmothilal Nov 14, 2019

rmothilal Nov 14, 2019

rmothilal Nov 14, 2019

vgenev left a comment

jgeewax commented Nov 15, 2019

rmothilal commented Nov 15, 2019

jgeewax commented Nov 18, 2019

rmothilal commented Feb 11, 2020

lewisdaly commented Feb 19, 2020

CLAassistant commented Oct 25, 2020

Updated email-notifier to use Jest for testing. #75

Are you sure you want to change the base?

Updated email-notifier to use Jest for testing. #75

Conversation

jgeewax commented Nov 13, 2019

jgeewax commented Nov 13, 2019

rmothilal commented Nov 13, 2019

jgeewax commented Nov 13, 2019

jgeewax commented Nov 14, 2019

vgenev Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmothilal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgenev Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgenev left a comment

Choose a reason for hiding this comment

jgeewax commented Nov 15, 2019

rmothilal commented Nov 15, 2019

jgeewax commented Nov 18, 2019

rmothilal commented Feb 11, 2020

lewisdaly commented Feb 19, 2020

CLAassistant commented Oct 25, 2020

vgenev Nov 14, 2019 •

edited

Loading

rmothilal left a comment •

edited

Loading

vgenev Nov 14, 2019 •

edited

Loading