CPD-1653: Added f-logger changes to GBF, Updated unit tests to cover … #116

kevinrodrigues · 2018-02-12T16:33:23Z

No description provided.

…changes.

DamianMullins · 2018-02-13T10:29:19Z

test/config.test.js

+        expect(config.logger).toBeDefined();
+    });
+
+    describe('`dir` it should exist', () => {


Introducing a different structure into the tests here — I know we discussed this before and agreed that we should hold a discussion on how best to structure tests but can we hold off on changing parts of the test files for now? It's a bit confusing when scanning through the file.

If/when we decide to change this structure I think it should all be done in a single PR, what do you think?

Oops, I will update it. I think we should refactor whatever we touch if/when we decide we want to change the structure, instead of doing massive changes in one PR?

There aren't that many tests so it might be OK to do the whole lot, depends how much we want to change the structure I guess.

DamianMullins · 2018-02-13T10:29:47Z

test/config.test.js

@@ -270,6 +270,37 @@ describe('javascript config', () => {

 });

+
+describe('`logger` config', () => {
+    it('should exist', () => {


There are no tests to check that the values can be updated?

DamianMullins · 2018-02-13T10:30:40Z

test/config.test.js

+    });
+
+    describe('`dir` it should exist', () => {
+        expect(config.logger.dir).toBeDefined();


Doesn't the following test already cover the defined scenario? i.e. it'll fail if it doesn't match 'JavaScript'.

This one is different to the test below. The idea here is that it will fail should the object property dir not exist. The second one is more of a value check, but I know what you mean, the value wouldn't pass if the dir isn't there in the first place so having it fail before that check makes it easier to spot what the actual fix is, what do you think?

DamianMullins · 2018-02-13T10:50:08Z

tasks/logger-file.js

+ */
+
+gulp.task('logger-file-create', () => {
+    const fullFilePath = `${pathBuilder.jsErrorLoggerSubDir}/${config.logger.file}`;


You should be able to use the gulp-file module combined with gulp.dest in order to keep the code running inside the pipe stream.

const file = require('gulp-file'); gulp.task('test', () => file(config.logger.file, '', { src: true }) .pipe(gulp.dest(pathBuilder.jsErrorLoggerDir)) );

This way there is no need to do the synchronous directory operations (gulp.dest handles that already). It also means you don't need as many config options, unless it required that you need that level of granularity?

ashleynolan · 2018-02-13T13:52:00Z

config.js

+         * f-logger-related variables
+         */
+        logger: {
+            dir: 'JavaScript',


What path does the js-error.js live at? Is it at somethign like /JavaScript/Shared/js-error.js?

Only wondering as that looks like a different directory to the rest of the JS on the site?

Can it not live in the same JS directory?

Caught up with Simon on this and there are a few areas that require this path to be set like this (below), I think we should raise a ticket to correct these paths and then we could update this so it lives in the JS folder, what do you think?

Ha-proxy NXlog Publicweb CW Kibana

Does Publicweb and hit the global logging file then? Or is it just some sort of side-affect…

Would be good to update anywhere that needs it too though – having JS in two places just adds confusion for people new to the project as the file will just get lost otherwise.

Have removed this. I'll update the jira so we can update the paths for all the other utilities that used the old path.

ashleynolan · 2018-02-13T13:54:04Z

tasks/logger-file.js

+ *
+ */
+
+gulp.task('logger-file-create', () => {


We've used a slightly different naming convention for a lot of the other tasks. Might make more sense if this was called logger:createFile or something like that.

Yup will update.

kevinrodrigues · 2018-02-14T09:42:42Z

More of question here but is it worth adding pre-commit hooks which will run unit tests and linting / anything else before we commit and push things, just to avoid pushing things that won't pass on travis?

ashleynolan · 2018-02-14T10:18:14Z

@kevinrodrigues Yeh, that'd be a good idea. Could save a bit of time when dev-ing he work. The other thing that I find useful is to hook up your editor to the eslint config – that way you get the warnings as you dev too

kevinrodrigues · 2018-02-14T10:21:29Z

@ashleynolan Yeah I have it hooked up but the error only highlighted after I committed and pushed (joys of WebStorm) 🐌

DamianMullins · 2018-02-15T08:25:53Z

tasks/logger-file.js

+ */
+
+gulp.task('logger:createFile', () => file(config.logger.file, '', { src: true })
+    .pipe(gulpif(!fs.existsSync(`${pathBuilder.jsErrorLoggerSubDir}/${config.logger.file}`),


In what scenario would the file already exist?

If the gulp task runs for the first time then the file would be created, the second time round the file won't be created. The reason the check is there is to prevent gulp from erroring in the scenario where the file might already exist e.g if the task has already run once already inside a project.

It's the sync bit which concerns me really, feels like we're wedging synchronous code into asynchronous streams. It's probably fine in this instance as it's such a small task 🎲

Have done some reading and this is the recommended way accorsing to SO, so I am happy 😸

DamianMullins · 2018-02-15T08:26:29Z

pathBuilder.js

@@ -14,6 +14,9 @@ const buildPaths = config => {
        jsSrcDir: `${config.assetSrcDir}/${config.js.jsDir}`,
        jsDistDir: `${config.assetDistDir}/${config.js.jsDir}`,

+        jsErrorLoggerDir: `${config.webRootDir}/${config.logger.dir}`,


Do you need this path anymore?

DamianMullins · 2018-02-15T08:31:55Z

test/config.test.js

+    const subDir = 'A sub directory';
+    const file = 'A file';
+
+    beforeEach(() => {


I personally find this style harder to follow than the existing tests. When everything is declared within a single file you don't have to go scanning back up to see if there is a beforeEach block. It doesn't add much value that I can see?

At least before the tests act more like documentation; someone can find the property they are interested in and see exactly how the properties are updated at a glance

Also, are we not waiting until we agree on a better to start rewriting the tests? Seems like we're introducing inconsistency because we disagree on the style. Wouldn't it be better to get on the same page before updating the tests?

Hi @DamianMullins I totally agree about not rewriting the tests until we agree on a structure which is why I have reverted them, but the reason I've added the beforeEach is because my tests fail on the .toBeDefined scenario if the config.update isn't updated before the second and the remaining IT block statements. I have removed the nested describes as we agreed but without repeating and updating the config multiple times for each IT and to keep things DRY I used beforeEach so as to not repeat code multiple times to make it more readable & maintainable. If you would prefer me to add them to each IT instead happy to do so?

OK, so it sounds like you have another issue in the tests, I'll have a quick look this afternoon so I fully understand the issue.

As for the comments about DRY, I'm always on the fence with applying the principle in unit tests as it tends to lead towards code which harder to reason about. Let's arrange something soon so we can discuss and put some examples together?

@simonsandersje Sounds like we need that doodle to sort out the next UI Guild 😉

@kevinrodrigues the issue you're having with the config value on the second run is because you need to add a line to the update() function which allows the property to update correctly.

logger: Object.assign(config.logger, options.logger)

And then the tests can be written in the same way:

describe('`logger` config', () => { it('should exist', () => { expect(config.logger).toBeDefined(); }); it('`dir` should be set', () => { expect(config.logger.dir).toBe('js/shared'); }); it('`dir` can be updated', () => { // Arrange const dir = 'logger/dir'; // Act config.update({ logger: { dir } }); // Assert expect(config.logger.dir).toBe(dir); }); it('`file` should be set', () => { expect(config.logger.file).toBe('js-error.js'); }); it('`file` can be updated', () => { // Arrange const file = 'logger.js'; // Act config.update({ logger: { file } }); // Assert expect(config.logger.file).toBe(file); }); });

Where are you updating the value in the code above? In the example I posted, the tests explicitly update the original value.

Why not keep it the same as everything else in the test file? I don't understand the need to make it different at the moment?

Sorry that I'm going on about this! 🐙

simonsandersje · 2018-02-15T12:06:26Z

config.js

+         */
+        logger: {
+            dir: 'js',
+            subDir: 'Shared',


Lower case "shared"

DamianMullins · 2018-02-16T09:07:12Z

config.js

+         * f-logger-related variables
+         */
+        logger: {
+            dir: 'js',


Why not just have dir? I don't think subDir is needed anymore?

PathBuilder.js requires it. Do you mean remove it and just place the logger file inside the js folder?

jsErrorLoggerSubDir: `${config.webRootDir}/${config.logger.dir}/${config.logger.subDir}`

Just change config.logger.dir to "js/shared"?

Cool updated now.

DamianMullins · 2018-02-16T09:14:10Z

test/config.test.js

+    // `logger.file`
+    it('`file` should exist', () => {
+        // Assert
+        expect(config.logger.subDir).toBeDefined();


Should be config.logger.file.

DamianMullins · 2018-02-16T11:40:02Z

README.md

@@ -307,6 +314,11 @@ Here is the outline of the configuration options, descriptions of each are below
        usePackageVersion,
        stripDebug
    },
+    logger: {


Need to add a description of these new properties to the docs below.

DamianMullins

Sorry about the constant pestering from me 🎆

ashleynolan · 2018-02-16T13:25:13Z

@DamianMullins Never apologise for that – just means @kevinrodrigues has to do the same on your PRs 😉

kevinrodrigues · 2018-02-16T13:31:46Z

https://m.popkey.co/df0a33/8wMw_f-maxage-0.gif

kevinrodrigues requested review from DamianMullins, ashleynolan and simonsandersje February 12, 2018 16:33

kevinrodrigues requested a review from a team as a code owner February 12, 2018 16:33

kevinrodrigues added 2 commits February 12, 2018 16:33

CPD-1653: Added f-logger changes to GBF, Updated unit tests to cover …

2c67c69

…changes.

CPD-1653: Removing console logs and fixing global require statement.

1d0e15f

DamianMullins reviewed Feb 13, 2018

View reviewed changes

ashleynolan reviewed Feb 13, 2018

View reviewed changes

kevinrodrigues added 4 commits February 13, 2018 16:35

CPD-1653: PR changes.

b7b2c05

CPD-1653: Updated failing tests.

fd1d3d7

CPD-1653: Added Assert comments to expect methods.

2959a57

CPD-1653: Fixed ES lint errors.

a1f93eb

DamianMullins reviewed Feb 15, 2018

View reviewed changes

CPD-1653: PR comments.

0a7df3d

simonsandersje reviewed Feb 15, 2018

View reviewed changes

CPD-1653: PR comments.

0b1a9a0

DamianMullins reviewed Feb 16, 2018

View reviewed changes

kevinrodrigues added 2 commits February 16, 2018 10:34

CPD-1653: PR comments.

6be46cb

CPD-1653: PR comments.

94c6ea9

DamianMullins reviewed Feb 16, 2018

View reviewed changes

CPD-1653: Updated readme.

30ce16b

DamianMullins approved these changes Feb 16, 2018

View reviewed changes

ashleynolan approved these changes Feb 16, 2018

View reviewed changes

kevinrodrigues merged commit 07f4ee9 into master Feb 16, 2018

kevinrodrigues deleted the CPD-1653-js-logging-component branch February 16, 2018 13:33

lcwj3 mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities lcwj3/gulp-build-fozzie#44

Open

CPD-1653: Added f-logger changes to GBF, Updated unit tests to cover … #116

CPD-1653: Added f-logger changes to GBF, Updated unit tests to cover … #116

Conversation

kevinrodrigues commented Feb 12, 2018

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

kevinrodrigues Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

DamianMullins Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

ashleynolan Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleynolan Feb 13, 2018 • 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

kevinrodrigues commented Feb 14, 2018

ashleynolan commented Feb 14, 2018

kevinrodrigues commented Feb 14, 2018

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

kevinrodrigues Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DamianMullins Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DamianMullins Feb 16, 2018 • 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

kevinrodrigues Feb 16, 2018 • 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

DamianMullins left a comment

Choose a reason for hiding this comment

ashleynolan commented Feb 16, 2018

kevinrodrigues commented Feb 16, 2018

kevinrodrigues Feb 13, 2018 •

edited

Loading

DamianMullins Feb 13, 2018 •

edited

Loading

ashleynolan Feb 13, 2018 •

edited

Loading

ashleynolan Feb 13, 2018 •

edited

Loading

kevinrodrigues Feb 15, 2018 •

edited

Loading

DamianMullins Feb 16, 2018 •

edited

Loading

DamianMullins Feb 16, 2018 •

edited

Loading

kevinrodrigues Feb 16, 2018 •

edited

Loading