Repository: codecov/uploader
Pull request #557 (2021-12-30 13:08:20)

fix: use logging class for most verbose calls
All operations that should depend on the set logging level must go through UploadLogger. The free functions are still required e.g. for things like `verbose(`Start of uploader: ${start}...`, realArgs.verbose)` running outside of `main`, so I kept them.

Consistently using `UploadLogger.verbose` is IMO more readable and makes it clear, that logging levels are used. If the free functions are to be preferred, the class can be removed.

To avoid mistakes with the free `verbose` remove the default Argument as that doesn't make sense anyway and is the source of current bugs. Those are fixed by this change as they all go through `UploadLogger` now.

@drazisil-codecov This is basically the all-round fix for https://github.com/codecov/uploader/pull/533#issuecomment-1002999737

The issue was likely introduced by some refactoring as evident in the test suite: `Logger Helper - Legacy log() tests'`

lgtm-com[bot]: This pull request **introduces 1 alert** when merging 6020374d30002c2f484ca2f012d59c34ae8653f6 into 4110ea7c2f67130e18a6cb1843845c35cf3deb89 - [view on LGTM.com](https://lgtm.com/projects/g/codecov/uploader/rev/pr-30a79542101a70f8e14d54892a093a054621a4e7)

**new alerts:**

* 1 for Unused variable, import, function or class

Patch:
diff --git a/bin/codecov.ts b/bin/codecov.ts
index edf1a02..1d2eef8 100755
--- a/bin/codecov.ts
+++ b/bin/codecov.ts
@@ -15,7 +15,7 @@ const realArgs = argv.argv
 
 const start = Date.now()
 
-verbose(`Start of uploader: ${start}...`, Boolean(realArgs.verbose))
+verbose(`Start of uploader: ${start}...`, realArgs.verbose)
 main(realArgs)
   .then(() => {
     const end = Date.now()
diff --git a/src/ci_providers/provider_githubactions.ts b/src/ci_providers/provider_githubactions.ts
index 3b8e238..4e34c26 100644
--- a/src/ci_providers/provider_githubactions.ts
+++ b/src/ci_providers/provider_githubactions.ts
@@ -4,7 +4,7 @@
 import { IServiceParams, UploaderEnvs, UploaderInputs } from '../types'
 
 import { runExternalProgram } from "../helpers/util"
-import { info, verbose } from '../helpers/logger'
+import { info, UploadLogger } from '../helpers/logger'
 
 export function detect(envs: UploaderEnvs): boolean {
   return Boolean(envs.GITHUB_ACTIONS)
@@ -74,7 +74,7 @@ function _getSHA(inputs: UploaderInputs): string {
   if (pr) {
     const mergeCommitRegex = /^[a-z0-9]{40} [a-z0-9]{40}$/
     const mergeCommitMessage = runExternalProgram('git', ['show', '--no-patch', '--format="%P"'])
-    verbose(`Handling PR with parent hash(es) '${mergeCommitMessage}' of current commit.`)
+    UploadLogger.verbose(`Handling PR with parent hash(es) '${mergeCommitMessage}' of current commit.`)
     if (mergeCommitRegex.exec(mergeCommitMessage)) {
       const mergeCommit = mergeCommitMessage.split(' ')[1]
       info(`    Fixing merge commit SHA ${commit} -> ${mergeCommit}`)
diff --git a/src/helpers/files.ts b/src/helpers/files.ts
index a3db7c3..58dedf9 100644
--- a/src/helpers/files.ts
+++ b/src/helpers/files.ts
@@ -4,7 +4,7 @@ import fs from 'fs'
 import { readFile } from 'fs/promises'
 import { posix as path } from 'path'
 import { UploaderArgs } from '../types'
-import { logError, UploadLogger, verbose } from './logger'
+import { logError, UploadLogger } from './logger'
 import { runExternalProgram } from './util'
 import micromatch from "micromatch";
 
@@ -232,7 +232,7 @@ export function getAllFiles(
   dirPath: string,
   args: UploaderArgs,
 ): string[] {
-  verbose(`Searching for files in ${dirPath}`, Boolean(args.verbose))
+  UploadLogger.verbose(`Searching for files in ${dirPath}`)
 
   const {
     stdout,
diff --git a/src/helpers/logger.ts b/src/helpers/logger.ts
index faa8050..0304d79 100644
--- a/src/helpers/logger.ts
+++ b/src/helpers/logger.ts
@@ -3,9 +3,6 @@
  * * Error
  * * Info
  * * Verbose
- * For the purposes of minimal refactor, I'm aliasing debug() to verbose()
- * I'm also mapping the log() options to the new calls and
- * deprecating log()
  */
 
 function _getTimestamp() {
@@ -15,10 +12,10 @@ function _getTimestamp() {
 /**
  *
  * @param {string} message - message to log
- * @param {boolean} shouldVerbose - pass the value of the verbose flag
+ * @param {boolean} shouldVerbose - value of the verbose flag
  * @return void
  */
-export function verbose(message: string, shouldVerbose = false): void {
+ export function verbose(message: string, shouldVerbose: boolean): void {
   if (shouldVerbose === true) {
     console.debug(`[${_getTimestamp()}] ['verbose'] ${message}`)
   }
@@ -50,21 +47,18 @@ export class UploadLogger {
     // Intentionally empty
   }
 
-  static init() {
+  static getInstance(): UploadLogger {
     if (!UploadLogger._instance) {
       UploadLogger._instance = new UploadLogger()
     }
+    return UploadLogger._instance;
   }
 
   static setLogLevel(level: string) {
-    UploadLogger.init()
-    UploadLogger._instance.logLevel = level
+    UploadLogger.getInstance().logLevel = level
   }
 
   static verbose(message: string) {
-    UploadLogger.init()
-    if (UploadLogger._instance.logLevel === 'verbose') {
-      console.log(`[${_getTimestamp()}] ['verbose'] ${message}`)
-    }
+    verbose(message, UploadLogger.getInstance().logLevel === 'verbose')
   }
 }
\ No newline at end of file
diff --git a/src/helpers/provider.ts b/src/helpers/provider.ts
index 359f1e5..7996c29 100644
--- a/src/helpers/provider.ts
+++ b/src/helpers/provider.ts
@@ -1,5 +1,5 @@
 import providers from '../ci_providers'
-import { info, logError, verbose } from '../helpers/logger'
+import { info, logError, UploadLogger } from '../helpers/logger'
 import { IServiceParams, UploaderInputs } from '../types'
 
 export function detectProvider(
@@ -41,15 +41,9 @@ export function walkProviders(inputs: UploaderInputs): IServiceParams {
   for (const provider of providers) {
     if (provider.detect(inputs.environment)) {
       info(`Detected ${provider.getServiceName()} as the CI provider.`)
-      verbose(
-        '-> Using the following env variables:',
-        Boolean(inputs.args.verbose),
-      )
+      UploadLogger.verbose('-> Using the following env variables:')
       for (const envVarName of provider.getEnvVarNames()) {
-        verbose(
-          `     ${envVarName}: ${inputs.environment[envVarName]}`,
-          Boolean(inputs.args.verbose),
-        )
+        UploadLogger.verbose(`     ${envVarName}: ${inputs.environment[envVarName]}`)
       }
       return provider.getServiceParams(inputs)
     }
diff --git a/src/helpers/token.ts b/src/helpers/token.ts
index f8032cf..8fd31bf 100644
--- a/src/helpers/token.ts
+++ b/src/helpers/token.ts
@@ -3,7 +3,7 @@ import yaml from 'js-yaml'
 import path from 'path'
 import { UploaderArgs, UploaderInputs } from '../types'
 import { DEFAULT_UPLOAD_HOST } from './constansts'
-import { info, logError, verbose } from './logger'
+import { info, logError, UploadLogger } from './logger'
 import { validateToken } from './validate'
 
 /**
@@ -26,7 +26,7 @@ export function getToken(inputs: UploaderInputs, projectRoot: string): string {
       // If this is self-hosted (-u is set), do not validate
       // This is because self-hosted can use a global upload token
       if (args.url !== DEFAULT_UPLOAD_HOST) {
-        verbose('Self-hosted install detected due to -u flag')
+        UploadLogger.verbose('Self-hosted install detected due to -u flag')
         info(`->  Token set by ${source}`)
         return token
       }
@@ -104,10 +104,7 @@ export function getTokenFromYaml(
           }
         }
       } catch (err) {
-        verbose(
-          `Error searching for upload token in ${filePath}: ${err}`,
-          Boolean(args.verbose),
-        )
+        UploadLogger.verbose(`Error searching for upload token in ${filePath}: ${err}`)
       }
     }
   }
diff --git a/src/index.ts b/src/index.ts
index 7641888..7f00162 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -5,7 +5,7 @@ import { version } from '../package.json'
 import * as validateHelpers from './helpers/validate'
 import { detectProvider } from './helpers/provider'
 import * as webHelpers from './helpers/web'
-import { info, logError, UploadLogger, verbose } from './helpers/logger'
+import { info, logError, UploadLogger } from './helpers/logger'
 import { getToken } from './helpers/token'
 import {
   cleanCoverageFilePaths,
@@ -136,7 +136,7 @@ export async function main(
   let uploadFile = ''
 
   if (!args.feature || args.feature.split(',').includes('network') === false) {
-    verbose('Start of network processing...', Boolean(args.verbose))
+    UploadLogger.verbose('Start of network processing...')
     let fileListing = ''
     try {
       fileListing = await getFileListing(projectRoot, args)
@@ -189,7 +189,7 @@ export async function main(
     throw new Error(noFilesError)
   }
 
-  verbose('End of network processing', Boolean(args.verbose))
+  UploadLogger.verbose('End of network processing')
   // #endregion
   // #region == Step 6: generate upload file
   // TODO: capture envs
@@ -251,9 +251,9 @@ export async function main(
 
   const buildParams = webHelpers.populateBuildParams(inputs, serviceParams)
 
-  verbose('Using the following upload parameters:', Boolean(args.verbose))
+  UploadLogger.verbose('Using the following upload parameters:')
   for (const parameter in buildParams) {
-    verbose(`${parameter}`, Boolean(args.verbose))
+    UploadLogger.verbose(`${parameter}`)
   }
 
   if (buildParams.slug !== '' && !buildParams.slug?.match(/\//)) {
@@ -271,19 +271,15 @@ export async function main(
       args.source || '',
     )}&token=*******&${query}`,
   )
-  verbose(
-    `Passed token was ${token.length} characters long`,
-    Boolean(args.verbose),
-  )
+  UploadLogger.verbose(`Passed token was ${token.length} characters long`)
   try {
-    verbose(
+    UploadLogger.verbose(
       `${uploadHost}/upload/v4?package=${webHelpers.getPackage(
         args.source || '',
       )}&${query}
         Content-Type: 'text/plain'
         Content-Encoding: 'gzip'
-        X-Reduced-Redundancy: 'false'`,
-      Boolean(args.verbose),
+        X-Reduced-Redundancy: 'false'`
     )
 
     const uploadURL = await webHelpers.uploadToCodecov(
@@ -294,13 +290,12 @@ export async function main(
       args,
     )
 
-    verbose(`Returned upload url: ${uploadURL}`, Boolean(args.verbose))
+    UploadLogger.verbose(`Returned upload url: ${uploadURL}`)
 
-    verbose(
+    UploadLogger.verbose(
       `${uploadURL.split('\n')[1]}
         Content-Type: 'text/plain'
         Content-Encoding: 'gzip'`,
-      Boolean(args.verbose),
     )
     const result = await webHelpers.uploadToCodecovPUT(
       uploadURL,


Test patch:
diff --git a/test/helpers/logger.test.ts b/test/helpers/logger.test.ts
index e8c7f42..a7c3177 100644
--- a/test/helpers/logger.test.ts
+++ b/test/helpers/logger.test.ts
@@ -1,46 +1,4 @@
-import { logError, info, verbose } from '../../src/helpers/logger'
-
-describe('Logger Helper - Legacy log() tests', () => {
-  afterEach(() => {
-    jest.clearAllMocks()
-  })
-
-  it('Should call logger with default options', () => {
-    // eslint-disable-next-line
-    jest.spyOn(console, 'log').mockImplementation(() => {})
-    info('message with no options')
-    expect(console.log).toHaveBeenCalledWith(
-      expect.stringMatching(/no options/),
-    )
-  })
-
-  it('Should not call logger with default options.level = debug and verbose not set', () => {
-    // eslint-disable-next-line
-    jest.spyOn(console, 'debug').mockImplementation(() => {})
-    verbose('message with debug level', undefined)
-    expect(console.debug).not.toHaveBeenCalled()
-  })
-
-  it('Should call logger with options.level = debug and verbose set', () => {
-    jest.spyOn(console, 'debug').mockImplementation(() => {
-      // Intentionally empty
-    })
-    verbose('message with debug level and verbose', true)
-    expect(console.debug).toHaveBeenCalledWith(
-      expect.stringMatching(/debug level and verbose/),
-    )
-  })
-
-  it('Should call logger with options.level = error', () => {
-    jest.spyOn(console, 'error').mockImplementation(() => {
-      // Intentionally empty
-    })
-    logError('message with error level')
-    expect(console.error).toHaveBeenCalledWith(
-      expect.stringMatching(/error level/),
-    )
-  })
-})
+import { logError, info, verbose, UploadLogger } from '../../src/helpers/logger'
 
 describe('Logging methods', () => {
   afterEach(() => {
@@ -48,9 +6,7 @@ describe('Logging methods', () => {
   })
 
   it('should call console.error() when error() is called', () => {
-    jest.spyOn(console, 'error').mockImplementation(() => {
-      // Intentionally empty
-    })
+    jest.spyOn(console, 'error').mockImplementation(() => { /* Intentionally empty */ })
     logError('this is a test error')
     expect(console.error).toHaveBeenCalledWith(
       expect.stringMatching(/\['error'\]/),
@@ -61,11 +17,26 @@ describe('Logging methods', () => {
     expect(console.error).toHaveBeenCalledWith(expect.stringMatching(/Z\]/))
   })
 
-  it('should call console.debug() when verbose() is called with shouldVerbose === true', () => {
-    jest.spyOn(console, 'debug').mockImplementation(() => {
-      // Intentionally empty
-    })
+  it('should not call console.debug() with default log level = info and verbose set to false', () => {
+    jest.spyOn(console, 'debug').mockImplementation(() => { /* Intentionally empty */ })
+    UploadLogger.setLogLevel('info')
+    verbose('this is a test verbose', false)
+    expect(console.debug).not.toHaveBeenCalled()
+  })
+
+  it('should call console.debug() with default log level = info and verbose set to true', () => {
+    jest.spyOn(console, 'debug').mockImplementation(() => { /* Intentionally empty */ })
+    UploadLogger.setLogLevel('info')
     verbose('this is a test verbose', true)
+    expect(console.debug).toHaveBeenCalledWith(
+      expect.stringMatching(/this is a test verbose/),
+    )
+  })
+
+  it('should call console.debug() when verbose() is called and log level = verbose', () => {
+    jest.spyOn(console, 'debug').mockImplementation(() => { /* Intentionally empty */ })
+    UploadLogger.setLogLevel('verbose')
+    UploadLogger.verbose('this is a test verbose')
     expect(console.debug).toHaveBeenCalledWith(
       expect.stringMatching(/\['verbose'\]/),
     )
@@ -75,18 +46,15 @@ describe('Logging methods', () => {
     expect(console.debug).toHaveBeenCalledWith(expect.stringMatching(/Z\]/))
   })
 
-  it('should not call console.debug() when verbose() is called without shouldVerbose', () => {
-    jest.spyOn(console, 'debug').mockImplementation(() => {
-      // Intentionally empty
-    })
-    verbose('this is a test verbose')
+  it('should not call console.debug() when verbose() is called and log level = info', () => {
+    jest.spyOn(console, 'debug').mockImplementation(() => { /* Intentionally empty */ })
+    UploadLogger.setLogLevel('info')
+    UploadLogger.verbose('this is a test verbose')
     expect(console.debug).not.toBeCalled()
   })
 
   it('should call console.log() when info() is called ', () => {
-    jest.spyOn(console, 'log').mockImplementation(() => {
-      // Intentionally empty
-    })
+    jest.spyOn(console, 'log').mockImplementation(() => { /* Intentionally empty */ })
     info('this is a test info')
     expect(console.log).toHaveBeenCalledWith(
       expect.stringMatching(/\['info'\]/),