Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DATAUP-765: assertify tests #3389

Merged
merged 4 commits into from
Dec 22, 2023
Merged

DATAUP-765: assertify tests #3389

merged 4 commits into from
Dec 22, 2023

Conversation

ialarmedalien
Copy link
Collaborator

@ialarmedalien ialarmedalien commented Nov 13, 2023

Description of PR purpose/changes

Convert existing self.assert... style-tests to the pytest-friendly assert versions.

Basically a big search-and-replace, aided by Ruff's auto-fix feature.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-765

  • Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

  • Details for how to test the PR:
  • Tests pass locally and in GitHub Actions
  • Changes available by spinning up a local narrative and navigating to X to see Y -- N/A

Dev Checklist:

  • My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • (JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
  • (Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook
  • Any dependent changes have been merged and published in downstream modules

Updating Version and Release Notes (if applicable)

@ialarmedalien ialarmedalien requested a review from briehl November 13, 2023 16:23
@ialarmedalien ialarmedalien self-assigned this Nov 13, 2023
@ialarmedalien ialarmedalien changed the base branch from develop to DATAUP-765_add_wsid_to_job November 13, 2023 16:26
@@ -1,14 +1,27 @@
import copy
from typing import Any
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in typing and method descriptions to help me whilst I was converting the tests.

lines.append(
{"is_error": 0, "line": "This is line {}".format(i + skip)}
)
lines.append({"is_error": 0, "line": f"This is line {i + skip}"})
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use f-string

@@ -74,7 +72,7 @@ def set_ws_name(ws_name):
]


@pytest.mark.parametrize("result,path,expected", get_result_sub_path_cases)
@pytest.mark.parametrize(("result", "path", "expected"), get_result_sub_path_cases)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruff prefers tuples to comma-separated strings, obviously!

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (4faae24) 29.35% compared to head (7840bf1) 29.33%.
Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3389      +/-   ##
===========================================
- Coverage    29.35%   29.33%   -0.02%     
===========================================
  Files          497      497              
  Lines        50569    50567       -2     
===========================================
- Hits         14843    14836       -7     
- Misses       35726    35731       +5     
Files Coverage Δ
src/biokbase/auth.py 100.00% <100.00%> (ø)
src/biokbase/narrative/jobs/appmanager.py 91.96% <100.00%> (ø)
src/biokbase/narrative/jobs/jobcomm.py 98.96% <100.00%> (ø)
src/biokbase/narrative/jobs/specmanager.py 98.88% <100.00%> (ø)
src/biokbase/narrative/contents/narrativeio.py 30.74% <66.66%> (ø)
src/biokbase/narrative/jobs/job.py 92.34% <98.30%> (ø)
src/biokbase/narrative/jobs/jobmanager.py 95.39% <95.23%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16b4c20...7840bf1. Read the comment docs.

@ialarmedalien ialarmedalien force-pushed the DATAUP-765_add_wsid_to_job branch from 4faae24 to b982e48 Compare November 16, 2023 14:39
Base automatically changed from DATAUP-765_add_wsid_to_job to develop November 16, 2023 17:35
Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly fine, just a mix of the previous PR and some auto gen test updates.

Was the change to kbaseGenericSetViewer.js intended this time? If not, maybe excise it for the next go around.

Otherwise, please do the rebasing and it's ready.

@@ -48,26 +48,32 @@ define([
this._super(options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are back! 😱
Was that intentional this time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Grrrrrr...

Removing some comments and an extraneous method; general tidying
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Thanks for removing that JS file change!

@briehl briehl merged commit af90f5f into develop Dec 22, 2023
7 of 10 checks passed
@briehl briehl deleted the DATAUP-765_assertify branch December 22, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants