Expand testing and documentation

[robert-cronin]

This is the testing PR, specifically for ensuring the Git operations and peer communications are deterministic.

PR will also add documentation to each class and interface.
Fixes #3
Fixes #62
Fixes #91

[CMCDragonkai]

BTW, for the compiled output, I'd recommend not using the minified output here.

Minification is only needed for web apps and the final GUI app.

For this sort of stuff, not-minifying allow us to more easily do diffs in our commits.

[CMCDragonkai]

Most of this is syntax changes right?

Can we merge the simple stuff first like syntax changes and then get to the meat of the stuff in the separate PR?

Also the number of files being changed in docs and dist always ends up blowing up the PR interface.

Is there a way we can not build the docs/dist changes until we need to? So the PRs here can just add functionality, and then just before we merge, we can add the build. Or even do that in a separate commit. It makes it very difficult to review the code with some much boilerplate noise.

[robert-cronin]

BTW, for the compiled output, I'd recommend not using the minified output here.

Minification is only needed for web apps and the final GUI app.

For this sort of stuff, not-minifying allow us to more easily do diffs in our commits.

Yeah sure, we can stop webpack from minifying with the following option:

optimization: {
    minimize: false
},

I will do this now and we can minify later

[robert-cronin]

Most of this is syntax changes right?

Can we merge the simple stuff first like syntax changes and then get to the meat of the stuff in the separate PR?

Also the number of files being changed in docs and dist always ends up blowing up the PR interface.

Is there a way we can not build the docs/dist changes until we need to? So the PRs here can just add functionality, and then just before we merge, we can add the build. Or even do that in a separate commit. It makes it very difficult to review the code with some much boilerplate noise.

Yeah I agree, the dist and docs folders clog up the review interface. We could always .gitignore those folders since they are generated output after all? Or probably the better option would be to do what we did last time and separate out the commits into a generated files commit for docs/dist etc and a source files commit for actual changes that need to be reviewed. I think I will maintain these two with rebasing.

[robert-cronin]

You can now exclude the generated files from a review based on a single commit and I will try to keep it this way with future changes 👍

[CMCDragonkai]

You can't gitignore it because the dist must exist for npm to pick it up. I reckon just push dist and docs to the final commit. But it may have to be in a commit outside of the MR to avoid polluting the interface.

…

On 22 July 2020 12:24:06 GMT+10:00, Robbie Cronin ***@***.***> wrote: > Most of this is syntax changes right? > > Can we merge the simple stuff first like syntax changes and then get to the meat of the stuff in the separate PR? > > Also the number of files being changed in docs and dist always ends up blowing up the PR interface. > > Is there a way we can not build the docs/dist changes until we need to? So the PRs here can just add functionality, and then just before we merge, we can add the build. Or even do that in a separate commit. It makes it very difficult to review the code with some much boilerplate noise. Yeah I agree, the dist and docs folders clog up the review interface. We could always `.gitignore` those folders since they are generated output after all? Or probably the better option would be to do what we did last time and separate out the commits into a `generated files` commit for docs/dist etc and a `source files` commit for actual changes that need to be reviewed. I think I will maintain these two with rebasing. -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #60 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[CMCDragonkai]

Oh this works?

…

On 22 July 2020 12:36:40 GMT+10:00, Robbie Cronin ***@***.***> wrote: You can now exclude the generated files from a review based on a single commit and I will try to keep it this way with future changes :+1: -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #60 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[CMCDragonkai]

What's the relationship between this PR and #73? Should this be merged first?

[robert-cronin]

#73 can be merged first, it is a separate addition that doesn't include any dedicated testing (the testing for that addition will be added in here)

[robert-cronin]

We now have 60 tests running to success, here are some noteworthy changes made to the library code:

• added in PK_LOG_PATH and PK_SOCKET_PATH environment variable options, purely for creating a separate agent environment for testing.
• separated the git backend and frontend into testable modules that don't depend on vaultManager.

These tests now also cover the agent and cli code

[robert-cronin]

This PR is now ready for review @CMCDragonkai

[robert-cronin]

I've noticed the jest tests fail in the pipeline because of the agent tests. This might be because the agent cannot connect in the nodejs image we use for the pipeline.

The tests are also throwing because there is no mock CA to test our secure connection.

I will create a new issue for this, I suspect the solution is using our new nix image instead of nodejs for our jest testing.

[robert-cronin]

Pipeline is successful, but hangs as described in #92. This is a specific issue with gitlab, so we can go ahead and merge.

[CMCDragonkai]

Nice! This will be useful as we are doing the training soon.

[CMCDragonkai]

@robert-cronin

added in PK_LOG_PATH and PK_SOCKET_PATH environment variable options, purely for creating a separate agent environment for testing.

These 2 env variables should be mentioned in the .env.example. I don't see it there.

Furthermore the shell.nix might want to set them according to project-local ./tmp directory.

[robert-cronin]

Im not sure it makes much sense to put them in .env, they are purely for testing and in that case we would need to include that file in a commit