Code Quality

[jankapunkt]

Some things I'd like to discuss and also work on is code quality and documentation:

• Tests and coverage (mocha, chai, sinon, istanbul)
• Linter
• API Docs
• Wiki, if desired
• README and badges
• GitHub Actions for CI and CD
• static analysis like semmle / lgtm for non-dependency vulnerabilities: https://github.com/node-oauth/node-oauth2-server/security/code-scanning

There are a few things to decide, like

• what linter do we want to use (I suggest something with zero config like prettier or standard)
• what doc generation (JSDoc) we want to use (I suggest also zero config like JSDoc)
• should master / main be push-protected?
• what should be required for CI to pass a basic pull request?
  • linter?
  • unit tests?
  • minimal required test coverage?
  • dependency audit via npm audit?
  • security analyses?

What else?

[HappyZombies]

I agree with a lot of these, but let me go down each bullet point and explain what I think.

• Tests and coverage (mocha, chai, sinon, istanbul)

  • The tests here currently use mocha and sinon, and they are all currently passing. More tests of course wouldn't hurt and even code coverage would be nice. (Also FYI, istanbul has been deprecated)

• Linter

  • Currently the package uses js-hint, which I will be honest I never used before lol. I am personally more familiar with eslint, but an interesting thing is that the dev branch uses eslint , but the version 5 typescript branch uses eslint. Is one better with TypeScript than the other? I think we should prepare for TypeScript when it comes for the linter, so which ever is better for that let's go with it.

• API Docs

  • JSDoc 100%

• Wiki, if desired

  • Sure but not right now, we got other stuff to do.

• README and badges

  • Important too but let's do other code quality stuff first -- this can be last thing we do.

• GitHub Actions for CI and CD

  • 100%! But again, being honest, I never used them before -- so a bit learning curve for me but don't worry I will figure it out.

• static analysis

  • 100% but again, should be something we do last after we get "everything else" done.

• should master / main be push-protected?

  • Yes. I think all features/bug fixes should go into a dev branch, we then make PRs that go into dev. Then when dev is ready for a version, we create a release branch, then a PR to merge to master with the new release. After that we tag and publish. What do you think?

Action items with priority:

Below is what I think should be done in order of importance.

• HIGH Mergining strategy: My idea is master is push protected, all bugs and features go to dev. When dev is ready we make a PR or a release branch, review it. Then the PR goes to master with the new version. Tag and npm publish. Or for ease we can just use master as 'dev' and cut release as we go.
• HIGH GitHub Actions for CI and CD: Simplest right now would be for unit tests to pass at least for now. Then we can add more stuff later on.
• HIGH Linter: I think we should go with Prettier.
• MEDIUM Static Analysis: Important but we should do the above first.
• MEDIUM There are currently unit tests and those are passing. We can update the packages and add more after we do the above.
• LOW API Docs
• LOW Readme and badges
• LOW Wiki

[jankapunkt]

Mergining strategy

I think the branching is good, we should always release into release so we can have CI run extra integration tests with all adapters before publishing. Dev can run only standard tests.

GitHub Action

I have experience with GH Actions and can provide a PR.

Linter

I have no experience with linter and typescript so maybe someone else who is more experienced in this combo starts a PR?

Static Analysis

Yes I'm good with that

API Docs

I can provide a PR later

Readme and badges and wiki

let's focus on that later

[HappyZombies]

Sounds good, I will clean up the dev branch and add protection to master so no one can push directly.

Sadly we lose whatever was on dev but worse case we just look at the old dev branch from the original repo.

[HappyZombies]

@jankapunkt should development be the default branch? I feel like doing so would kinda make master redundant...which then begs the question of why even have a development branch 🤷

[jankapunkt]

I have a clear opinion on this: master is always reflecting the latest stable release, so after a release being published with no bigger issues we should merge it to master. All development effort is going into development and release candidates are merged into release. In such a case master remains the base branch, because it reflects the latest production-stable version, no matter what mess has been created on other branches and this is why I think it should be push-protected.

[jankapunkt]

@HappyZombies I added a PR regarding branching and contribution guidelines and I would ask to you to work on the maintainers part a bit (which is more closely related to the branching strategy) and let me know what you think of the current state.

[jwerre]

I'm also a fan of:

• mocha, chai or should
• eslint
• jsdocs

[jankapunkt]

@jwerre good thing is that current tests already use mocha and chai so there is not much to change on their side. I already added NYC for coverage in 10 min or so

[jwerre]

I just realized the should.js is the main the assertion library but the repo has been archived! I wonder if continuing to use it would be the best way to future-proof. What do you guys think? Do you think we could just use Chai's should interface as a simple swap in replacement. e.g.:

// var should = require('should'); // old way
const should = require('chai').should()

What a lot of people don't like about should is that it extends the Object.prototype. I've used should for years and never had any problem but I get the complaint. That said it could be a lot of work change all the tests.

[jankapunkt]

I personally love the Chai asserter and I think together with Mocha it has the widest spread of expertise so many people should be familiar with it (yes very meta).

I think we should focus on this before we do the other issues because this is a fundamental thing for our CI and future contributions

[jankapunkt]

What about using expect if we need to rewrite them anyway?

[jwerre]

I don't think you'd need to rewrite anything if you just use Chai's should interface. I'll check it out now and see what happens.

[jwerre]

I just did a find for should = require('should'); and replace with should = require('chai').should() in the entire test/unit directory and all but one test passed.

[jankapunkt]

Another code quality tool we could leverage to prevent secrets being pushed: https://github.com/trufflesecurity/truffleHog

For tests we could set some fuzzing tools in perspective, that run before a new release may be published: https://github.com/google/oss-fuzz

[jwerre]

Are we going to move forward with ESLint? Perhaps we should start a new issue for this topic and discuss configuration?

[HappyZombies]

I think we should go for ESLint, I thought about prettier, but having it find code errors will help us catch those undefined errors quickly.

In the original project, they started using ESLint but, never got merged to master. Perhaps this is something we can benefit from?

https://github.com/oauthjs/node-oauth2-server/blob/dev/.eslintrc

[jankapunkt]

I agree let's create a separate issue and discuss a potential eslint config

[jankapunkt]

I think we should set Pull Requests to require at least one passing review to be merged, for critical things like new features or feature changes even two

[HappyZombies]

@jankapunkt I agree, I will make this change..if i can? lol

[jankapunkt]

@HappyZombies there should be a setting somewhere that pull requests need approval

[jankapunkt]

@HappyZombies I think it's part of branch protection rules https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

[HappyZombies]

Ah I see, I need to make the rule for development branch specifically, got it thanks!

[HappyZombies]

@jankapunkt Done. btw do you not have access to those settings? I can up your privileges if needed.

[oklas]

What about actually merge or rebase for merge strategy? Each has its own pros and cons. The merge produce too obfuscated network of branches in history while rebase gives a linear chain of commits.

[jankapunkt]

@oklas correct but I also messed up a complete project with rebasing so I stick mostly with merge or squash merge. In which situation I would choose a rebase over merge when approving a pull request?

[HappyZombies]

I'm okay with merging and squash merging. Rebasing...eh, but personal preference (plus I think more people know about merging, but this shouldn't be a factor in us deciding this)

[oklas]

I moved details and discussion about situations with rebase and merge let's refer #31. ESLint also moved to its ticket. The next high question is CI/CD.

[jwerre]

Is it safe to delete the travis.yml?

[jwerre]

@jankapunkt I was looking at the workflow you set up and I see that it's only testing on node 12. I wonder if it would be better to test in multiple versions? What do you think? Also, see previous comment.

[HappyZombies]

@jwerre let's take the Node version convo to #34

[HappyZombies]

Once #18 is merged in, any PR moving forward (not the current ones) will have to abide by the commit and PR conventions.

[Uzlopak]

Also i want to mention that the integration tests are imho basically pointless.

Currently everything is mocked with pointless stubs. How you want to check if the scope is valid, if the method always returns a specified return value.

It would be better to implement an in memory model. And write unit tests which check the correct behavior of the memory model and then run tests against it. Thus showing us some real issues.

[HappyZombies]

Closing this issue as other individual items have been made and I believe we have addressed all we could here.