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

split test functions in gha #2321

Merged
merged 1 commit into from Jun 7, 2021
Merged

split test functions in gha #2321

merged 1 commit into from Jun 7, 2021

Conversation

briehl
Copy link
Member

@briehl briehl commented Jun 7, 2021

Description of PR purpose/changes

Separates the test actions into unit test and integration test runs, so they can be more easily checked to see what passes and what fails. It maybe makes things take a little longer as the app has to get built multiple times, but I think it'll be helpful for seeing at a glance what's failing.

This came up from seeing a new batch of dependabot PRs which all failed miserably. This is really due to dependabot using a separate fork for submitting PRs - so another option might be to have the integration tests skip / fail immediately if there's no token available. Right now, they continue and just time out all over the place as they're running tokenless. The unit tests should be fine without authentication, and in fact, that's preferred.

Jira Ticket / Issue

e.g. https://kbase-jira.atlassian.net/browse/DATAUP-X

  • [n/a] 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

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
  • Any dependent changes have been merged and published in downstream modules
  • [n/a] (JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
  • [n/a] (Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook

Updating Version and Release Notes (if applicable)

@sonarcloud
Copy link

sonarcloud bot commented Jun 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #2321 (b599232) into develop (e38ffd5) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2321      +/-   ##
===========================================
+ Coverage    12.90%   12.91%   +0.01%     
===========================================
  Files          415      415              
  Lines        44581    44581              
===========================================
+ Hits          5752     5757       +5     
+ Misses       38829    38824       -5     
Impacted Files Coverage Δ
...-extension/static/kbase/js/util/bootstrapSearch.js 95.08% <0.00%> (+3.27%) ⬆️
kbase-extension/static/kbase/js/common/unodep.js 49.18% <0.00%> (+4.91%) ⬆️

Continue to review full report at Codecov.

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

@ialarmedalien
Copy link
Collaborator

Are you already on the case of the codecov failure? PR looks fine otherwise.

@briehl
Copy link
Member Author

briehl commented Jun 7, 2021

Yeah, I'm on it. It's strange. I think it's a secrets issue, but I'm not sure how.

I also noticed that the Python coverage file setup is wrong - it's making the wrong file! This led me down a little rabbit hole of switching from nosetests -> pytest in the Python stack, but that should probably be a different PR. Happily, that changeover was pretty much just swapping one for the other.

@briehl
Copy link
Member Author

briehl commented Jun 7, 2021

Looks like it's a new issue with Codecov.
codecov/codecov-action#330 (opened yesterday) seems to be the same thing, public repos throwing that error.

I still want to go through with fixing the Python coverage stuff. But I'll do that in another PR.

Copy link
Collaborator

@ialarmedalien ialarmedalien left a comment

Choose a reason for hiding this comment

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

These changes look fine; with any luck the codecov people will fix their side of the coverage problem so that the unit test workflow will pass.

@briehl briehl merged commit 08b0c1a into develop Jun 7, 2021
@briehl briehl deleted the split-test-setup branch June 7, 2021 17:59
@briehl briehl mentioned this pull request Jun 8, 2021
12 tasks
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.

None yet

2 participants