DEFRA / water-abstraction-team

Guides, info and issue management for the Water Abstraction Team
Other
1 stars 0 forks source link

Check for console.log() in CI #94

Closed Cruikshanks closed 3 months ago

Cruikshanks commented 1 year ago

Whilst pairing on a recent change we added a bunch of console.log() to see what was happening. But we almost forgot to remove them before squashing the change into main.

Any logging we do should be via our app/lib/[*]-notifier.lib.js modules to ensure the format is consistent. So, no console.log() should exist in a change.

It would be great if our CI could catch these in future to prevent us from making this mistake. This would be a timely enhancement as we are about to have a change of personnel on the team.

We already do a similar check for .only() in our tests.

      # Before we do anything, check we haven't accidentally left any `describe.only()` or `it.only(` statements in the
      # tests
      #
      # Reworking of https://stackoverflow.com/a/21788642/6117745
      - name: Temporary tag check
        run: |
          ! grep -R 'describe.only(\|it.only(' test 

🤞 this wouldn't be too difficult to copy and rework for console.log().

Note 1 - There does exist one valid console.log() in app/server.js. This is there to log unhandled exceptions. Theoretically, we should never hit this line but we might have broken something and an error could be thrown before our notifier and error-handling plugins have been registered. So, we need it to remain but there is nothing to stop us from changing it to console.error(). 😉

Note 2 - This only applies to water-abstraction-system. We know the legacy code is littered with console.log() and we don't intend to go and fix all those instances.

rvsiyad commented 3 months ago