cisagov / tpt-reports

Process to build and distribute Technical Phishing Test (TPT) reports
Creative Commons Zero v1.0 Universal
4 stars 0 forks source link

Add test function for loading JSON file #51

Closed shulmanj closed 11 months ago

shulmanj commented 11 months ago

๐Ÿ—ฃ Description

This PR resolves issue #20 and creates a test case for the load_json_file function in tpt_reports.py.

๐Ÿ’ญ Motivation and context

This will add a test case to an untested function.

๐Ÿงช Testing

All automated tests, including this new test, were successful.

โœ… Pre-approval checklist

โœ… Pre-merge checklist

โœ… Post-merge checklist

coveralls commented 11 months ago

Pull Request Test Coverage Report for Build 6316974395


Totals Coverage Status
Change from base Build 6316899254: 0.0%
Covered Lines: 0
Relevant Lines: 0

๐Ÿ’› - Coveralls
dav3r commented 11 months ago

Also, the "Testing" section of this PR's description is blank. You should always include some explanation of how the changes were tested, even it's something simple like "All automated tests, including this new test, were successful."

dav3r commented 11 months ago

@nickviola Are you planning to start versioning this repo soon? I know we had some discussion about that recently.

The last several PRs in this repo have had "Finalize version" and "Create release" unchecked in the PR description. You really should be doing those things (happy to help explain if you're unsure), but when you're not doing them, they should be removed from the PR description.

schmelz21 commented 11 months ago

@dav3r , @nickviola - Dave, glad you brought this up. In the future we will look to modify the release/version checkboxes appropriately. Oversight on our part.

As for this project, it seems we will be looking at version 1.0.0. and first release as there are no new/worthy requirements coming our way until we hit production at this point. just a few remaining adjustments.

For your team, would it be expected for us to create a standalone PR to make the version modification in the code and create the release, or just add into our last PR that closes out the effort. If there is even more to it, we'd be happy to sync up t discuss. thanks.

dav3r commented 11 months ago

@schmelz21, @nickviola - For our Python repos, you should either make a new release with every PR or else group several related PRs into a single release. When a repo is relatively new (e.g. hasn't reached v1.0 yet), it's perfectly fine to have a lot of small releases (0.0.1, 0.0.2, 0.1.0, 0.1.1, etc.)

Since this repo's version has never been changed from the skeleton (still at 0.0.1), here's what I recommend doing:

You can also feel free to create pre-release releases during your testing and PR approval process if they will be helpful to you, but those are optional and will depend on your development and deployment process. Even if you don't create pre-releases, we strongly encourage creating finalized releases after your PR (or group of related PRs) gets merged.

Please let me know if you have any questions about any of this.

For your team, would it be expected for us to create a standalone PR to make the version modification in the code and create the release, or just add into our last PR that closes out the effort.

Generally, you should avoid pull requests that only change the version. It's preferable to include the version change in with the PR that contains a functionality change or bug fix.

schmelz21 commented 11 months ago

@dav3r - Appreciate such thorough directions. the team will review and begin to aquire these rules into our dev / deploy processes. Thanks.

nickviola commented 11 months ago

@schmelz21, @nickviola - For our Python repos, you should either make a new release with every PR or else group several related PRs into a single release. When a repo is relatively new (e.g. hasn't reached v1.0 yet), it's perfectly fine to have a lot of small releases (0.0.1, 0.0.2, 0.1.0, 0.1.1, etc.)

Since this repo's version has never been changed from the skeleton (still at 0.0.1), here's what I recommend doing:

  • On the next PR for this repo, bump the version appropriately, based on the changes in the PR, see https://semver.org/ for details, but these are the basic rules:

    • MAJOR version when you make incompatible API changes
    • MINOR version when you add functionality in a backward compatible manner
    • PATCH version when you make backward compatible bug fixes
  • Here's how to initially set the version, depending on the level of change in the PR:

    • ./bump_version.sh patch (would change version from 0.0.1 to 0.0.2)
    • ./bump_version.sh minor (would change version from 0.0.1 to 0.1.0)
    • ./bump_version.sh major (would change version from 0.0.1 to 1.0.0)
  • After you bump the version in the previous step, you should immediately bump the version for a prerelease. This is the version that you will use for your testing and during your PR approval process:

    • ./bump_version.sh prerelease (would change version to 0.0.2-rc.1, 0.1.0-rc.1, or 1.0.0-rc.1 depending on which of the version bumps you did in the previous step)
  • During your testing and or PR approval process, feel free to bump the prerelease version as often as you deem necessary.
  • Once your PR has been fully tested and approved, the last step is to finalize the version (this is the pre-merge checklist item that we include in the PR template):

    • ./bump_version.sh finalize (would change version to 0.0.2, 0.1.0, or 1.0.0 depending which level you bumped to initially)
  • After the PR has been merged, you would then create a release in GitHub that matches your version, i.e. if the finalized version is 0.0.2, then the corresponding release should be called v0.0.2 (this is the post-merge checklist item that we include in the PR template).

You can also feel free to create pre-release releases during your testing and PR approval process if they will be helpful to you, but those are optional and will depend on your development and deployment process. Even if you don't create pre-releases, we strongly encourage creating finalized releases after your PR (or group of related PRs) gets merged.

Please let me know if you have any questions about any of this.

For your team, would it be expected for us to create a standalone PR to make the version modification in the code and create the release, or just add into our last PR that closes out the effort.

Generally, you should avoid pull requests that only change the version. It's preferable to include the version change in with the PR that contains a functionality change or bug fix.

Just catching up on this discussion @dav3r and @schmelz21. Thank you for the detailed info @dav3r. Since we have been in the pre-release stage, we were unsure about when to start versioning, so this is helpful. We'll review/test and begin to work that into the dev process. Thanks again!