Fix: CI tests failing

[linux-nerd]

Details

Description

I tried solving this issue in my PR #135, but it seems it requires some discussion. So, instead of blocking that PR I created a new one.

• Removed dependency of .env file to point to correct urls (also being discussed in process.env #81)
• Updated the snapshots
• Removed App.test.js file as App.js defines the routes of the application.
• Change analyzerMode to static to prevent the analyzer server from starting
• Added test script in lint-staged

Unit test is failing for App.test.js. I have a strong feeling that the test is failing because of @react/router. Their documentation does not say anything about testing with reach router. I would appreciate any help in this regards.

Failing test case -

 FAIL  src/containers/App/__tests__/App.test.js
  ● Console

    console.error node_modules/react-dom/cjs/react-dom.development.js:14389
      The above error occurred in the <RouterImpl> component:
          in RouterImpl (created by LocationProvider)
          in LocationProvider (created by Context.Consumer)
          in Location (created by Context.Consumer)
          in Router (at App.js:10)
          in App (created by I18n)
          in I18n (created by Translate(App))
          in Translate(App) (at App.test.js:6)

      React will try to recreate this component tree from scratch using the error boundary you provided, LocationProvider.
    console.error node_modules/react-dom/cjs/react-dom.development.js:14389
      The above error occurred in the <LocationProvider> component:
          in LocationProvider (created by Context.Consumer)
          in Location (created by Context.Consumer)
          in Router (at App.js:10)
          in App (created by I18n)
          in I18n (created by Translate(App))
          in Translate(App) (at App.test.js:6)

      Consider adding an error boundary to your tree to customize error handling behavior.
      Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

  ● renders the homepage

    TypeError: Cannot read property 'split' of undefined

Issue

#134

[crysfel]

Thanks, just create the config object

[crysfel]

Let's create a configuration file, where we get the value from the environment or assign a default value if empty: src/config/Constants.js

export default {
  contact: {
    EMAIL:  process.env.EMAIL ||  '[email protected]',
    SLACK_URL: process.env.SLACK_URL ||  '......',
  },
  social: {
    FB_URL: process.env.FACEBOOK_URL || 'https://www.facebook.com/codingcoachio/',
    INSTA_URL: process.env.INSTA_URL ||  'https://www.instagram.com/',
    TWITTER_URL: process.env.TWITTER_URL  || 'https://twitter.com/codingcoach_io',
  },
};

Then we will use the config object with the values from the env or the default.

[linux-nerd]

Hmm.. Good one. I was thinking of creating a constant file but never thought of mixing the env in the constants 👍

[linux-nerd]

Till now I have never unit tested the routing part of the application. App.js mostly defines the routes. Do you guys think its good to test that? And if yes, then what exactly should we test?

[crysfel]

@linux-nerd I don't think is worth testing the router, after all it's already tested in their own repo. Let's remove that test.

[crysfel]

It looks like everything went well in the build, but still is showing as failed. I think that's because the bundle analyzer is running an http server.

Maybe we can send a parameter in the CI to avoid running the analyzer there. I've seen that there's an option to add a custom script, we might be able to send a parameter to enable the analyzer whenever we would like to generate it, can you take a look?

https://github.com/Coding-Coach/coding-coach/blob/development/scripts/config-overrides.prod.js#L37

Maybe something like this:

if (process.argv[0] === 'analyze') {
  // Adds the bundle analyzer to inspect the production build
  config.plugins.push(new BundleAnalyzerPlugin());
}

Another option is to take a look at the parameters received by BundleAnalyzerPlugin and see if we can disable the http server and instead just saving the files in the disk.

[linux-nerd]

sure, I will look at it tomorrow :)

[arthurbuhl]

This should be test:staged instead of just test, right?
Also, we should have test script before git add, so that we make sure that git add only happens if the tests passed successfully.

[arthurbuhl]

Looks good to me. It would be great if we could merge this PR asap, so that all other PR related CI builds and the main dev build start working fine.

[emmabostian]

Seems like the build is passing now - do I need to do anything else?

[crysfel]

Awesome job! Thanks for taking the time to fix these issues!!