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

chore: Various cleanups I saw #2

Merged
merged 1 commit into from
Feb 16, 2022
Merged

chore: Various cleanups I saw #2

merged 1 commit into from
Feb 16, 2022

Conversation

CelticMajora
Copy link
Contributor

Description

  • Switches to using invertase's analysis github action(annotates on PR)
  • Switches to using zgosalvez/github-actions-report-lcov(comments on PR)
  • Deletes and gitignores a couple files

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@github-actions
Copy link

LCOV of commit 74dfb94 during alchemist #2

Summary coverage rate:
  lines......: 100.0% (344 of 344 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Comment on lines +26 to +32
- name: Report code coverage
uses: zgosalvez/github-actions-report-lcov@v1.4.0
with:
path: coverage/lcov.info
coverage-files: coverage/lcov.info
minimum-coverage: 100
github-token: ${{ secrets.GITHUB_TOKEN }}
artifact-name: code-coverage-report
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@definitelyokay Would love your input here. We had been using this action on charlatan and better_test_reporter to report coverage. Looking at very_good_coverage, it looks like it shares the functionality of gating based on a coverage threshold, but does not comment on the PR with the coverage report. The benefit very_good_coverage has over this is that it has built in exclude support. Right now in our other packages, we run a script to exclude the coverage by removing it from the lcov report.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting! I'm fine with using anything that works best for the package, though I think we might be able to defragment the landscape of these packages a bit of we submit a PR to VGV's action that adds the functionality we want instead :)

I wouldn't let that block this PR though!

Also, about the report:

  • We usually export the coverage report using a separate job that adds all coverage related artifacts. This might be what you were looking for if I'm not mistaken.
  • While the VGV coverage action doesn't add this report to the artifacts by default, it does tell you what lines are missing coverage if the action failed, courtesy of this PR I did a while ago. If this doesn't work as intended, I'd love to hear so! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeroen-meijer like the idea of adding the PR commenting functionality to the VGV action. I can take a look into that at some point, but probably don't have the bandwidth at the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've added some comments to some of our open source issues relating to this! Definitely recommend using whatever is working well for you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, thanks both of you! :)

Comment on lines +20 to +21
- name: Analyze project source
uses: invertase/github-action-dart-analyzer@v1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to make sure that there isn't anything extra for flutter analyze that is needed here. Will get back to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Poked around a bit online and I think dart analyze should be as strict or more strict than flutter analyze so I think I'd be comfortable using this action as it gives us the benefit of in-diff annotations of failures. What are other folks' thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this more, those in-diff annotations of failures are super cool! AFAIK dart analyze and flutter analyze should be basically the same since we have lint rules set up in analysis_options.yaml

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great!

@CelticMajora
Copy link
Contributor Author

Love the pana script and will probably try to get that running in our other packages 🎉

@jeroen-meijer
Copy link
Collaborator

LCOV of commit 74dfb94 during alchemist #2

Summary coverage rate:
  lines......: 100.0% (344 of 344 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Ok, coming back to my previous comment about our coverage package, I got to say, this is pretty sweet... ✨

@CelticMajora CelticMajora merged commit 2269f8d into main Feb 16, 2022
@CelticMajora CelticMajora deleted the various-cleanups branch February 16, 2022 16:18
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.

4 participants