Site-kit-wp: Tests don't fail when global data is unable to be loaded

Created on 30 Jul 2020  路  4Comments  路  Source: google/site-kit-wp

Bug Description

Currently, when data is unable to be resolved from our globals (usually only in tests where it isn't manually received), we log an error to the console.

https://github.com/google/site-kit-wp/blob/e4539d97a47fee9a21e6ef29e1a6bdd4d86d5379/assets/js/googlesitekit/datastore/site/info.js#L116-L119

However, this does not cause tests to fail where it should. We should make this throw an error, but only in development to make sure these things cause tests to fail as expected.


_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

  • Introduce a new throwOnError feature flag which is only enabled in development
  • Use this feature flag in places where console.error is currently only called to conditionally throw an Error instead of calling console.error

    • console.error should still be called if the feature flag is false (i.e. production)

Implementation Brief

  • Add a new throwOnError.enabled key to webpack.feature-flags.config.js
  • Update "could not load" errors to use the new feature flag

    • If enabled, throw an error with the same message, otherwise preserve the current console behavior

  • Fix failing tests as a result
    _Changing this for "Could not load core/site info" resulted in 6 failing tests (have not tried others)_
  • Set global.featureFlags in tests/js/setup-globals.js since Jest does not use Webpack, the plugin has to be "loaded" manually
  • Finish and merge https://github.com/google/site-kit-wp/pull/1853

QA Brief

Changelog entry

P0 Bug

All 4 comments

@aaemnnosttv Do we need to make this change? With #1770 such logs should make the tests fail already.

@felixarntz in that case, I suppose we don't need to change this. We can revisit this after that issue if needed 馃憤

Removing this from the milestone since it may not be necessary - let's wait for #1770 to see whether it is.

@aaemnnosttv I believe this is no longer relevant? I'll close it for now.

Was this page helpful?
0 / 5 - 0 ratings