Improve change detection for environments, expand unit tests

[Brad-Abrams]

This PR is superseded by PR #649

Environments unit test hid an error in Environments plugin; fixed

TLDR; removes a redundant/faulty check for changes in the environments plugin

Before:

Before this change there was a single unit test for environments. That test modelled 7 environments with a change in each.

Two of the seven changes (wait_timer, prevent_self_review) were detected by mergeDeep.compareDeep(); 5 were not detected.

The two changes that were detected were enough to lift the code execution past this point, deeper into the environments code which itself again uses it's own comparator() where this time it successfully finds the 7 changes.

It was theorized that if the unit test was decomposed into it's seven constituent pieces that only 2/7 would pass. This turned out to be true and is a better match to our lived experience with using environments in safe-settings:

Now on the theory that the first round of change detection was redundant, it was removed. This was accomplished by copying the sync method from diffable.js into environments.js and then removing the lines which failed to detect the changes. A new run of the unit tests, post change, passes 7/7:

But, what if the removed code wasn't really redundant? What if the existing config, and the desired config were the same; would environments.js now erroneously call GitHub to make a change when none was required? Nope:

After:

The original test with 7 environments and 7 changes bundled together was added back in alongside the decomposed tests. It also passes. Here are the final results:

Request:

I would be most appreciative of a review and merge of this change. I am available for any follow up questions which may arise.

[klutchell]

I'm going to give this PR a try, with some minor changes from https://github.com/github/safe-settings/pull/616 on top.

[Brad-Abrams]

I have a new PR inbound that will combine together this PR #646 and Kyle's PR #616. It just needs to navigate some internal processes first. It will additionally address issue #623 with a documentation update.

I also noticed that PR #630 relates to environments, however it has no material code change and so I've not taken it into consideration. ... I read it some more, and now I've taken it into consideration ... PR #630 stated that it was intended to address issue #628. I'm going to see what I can do about #628

[Brad-Abrams]

A new PR #649 supersedes this one, and combines in the changes from PR #616 as well.

I did look into PR #630 which mentions environments but has no material code changes, and the underlying issue #628. I may come back to it, however its of a lower priority and not included here.