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

test: add additional bad archive tests #1322

Merged
merged 3 commits into from
Aug 5, 2021
Merged

Conversation

P0intMaN
Copy link
Contributor

@P0intMaN P0intMaN commented Aug 5, 2021

Additional test_bad_* added. (fix #1305)

Copy link
Contributor

@BreadGenie BreadGenie left a comment

Choose a reason for hiding this comment

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

You need to commit the respective empty files too.
Take a look at #1285

You can create one using touch empty-file.<extension> in linux systems in the asset directory

@BreadGenie
Copy link
Contributor

BreadGenie commented Aug 5, 2021

You can enable pre-commit to run the linting checks before commits in your local machine

Run

pip install -r dev-requirements.txt
pre-commit install
pre-commit run -a

Also don't forget to commit according to https://conventionalcommits.org

@P0intMaN
Copy link
Contributor Author

P0intMaN commented Aug 5, 2021

Hello @BreadGenie,

Yes, I absolutely missed that part. So, I need to create a folder test/assets and inside it I would have to pass the blank files with name empty-file and with the matching extensions right?

@P0intMaN
Copy link
Contributor Author

P0intMaN commented Aug 5, 2021

You can enable pre-commit to run the linting checks before commits in your local machine

Run

pip install -r dev-requirements.txt
pre-commit install
pre-commit run -a

I was going to ask about this just now. Seeing all those red crosses got me worried.

@BreadGenie
Copy link
Contributor

So, I need to create a folder test/assets and inside it I would have to pass the blank files with name empty-file and with the matching extensions right?

Yep

@P0intMaN
Copy link
Contributor Author

P0intMaN commented Aug 5, 2021

Also don't forget to commit according to https://conventionalcommits.org

This commit is of type fix or test? I think this is a test type.

@BreadGenie
Copy link
Contributor

This commit is of type fix or test? I think this is a test type.

Yep.

@P0intMaN
Copy link
Contributor Author

P0intMaN commented Aug 5, 2021

ok cool! I am committing now. Pre commit showed passed for both the lintings.

@P0intMaN P0intMaN changed the title Add additional bad archive tests test: add additional bad archive tests Aug 5, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #1322 (f839ada) into main (0343ec0) will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1322      +/-   ##
==========================================
+ Coverage   78.89%   79.08%   +0.19%     
==========================================
  Files         271      271              
  Lines        4865     4881      +16     
  Branches      585      585              
==========================================
+ Hits         3838     3860      +22     
+ Misses        875      872       -3     
+ Partials      152      149       -3     
Flag Coverage Δ
longtests 79.08% <100.00%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/test_extractor.py 100.00% <100.00%> (ø)
cve_bin_tool/extractor.py 57.86% <0.00%> (+3.77%) ⬆️

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 0343ec0...f839ada. Read the comment docs.

@P0intMaN
Copy link
Contributor Author

P0intMaN commented Aug 5, 2021

here comes the fresh commit.

Copy link
Contributor

@BreadGenie BreadGenie left a comment

Choose a reason for hiding this comment

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

LGTM

nitpick: PR description could have fix #1305 instead of issue #1305

@BreadGenie
Copy link
Contributor

@P0intMaN you can join our gitter chat

it'd be much easier to get help there

@P0intMaN
Copy link
Contributor Author

P0intMaN commented Aug 5, 2021

it'd be much easier to get help there

Ok joining right away!
Thanks!

@terriko
Copy link
Contributor

terriko commented Aug 5, 2021

Not really sure why the one test in python 3.7 failed, but it looks like it could be a transient network error, so I'm going to re-run CI.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Looking good! Now that we've got 6 files instead of 2 we might want to refactor the filename stuff at the top so it's not so repetitive, but that doesn't have to happen in this pull request.

@terriko terriko merged commit 9159639 into intel:main Aug 5, 2021
@P0intMaN P0intMaN deleted the bad-archives branch August 27, 2021 04:12
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.

Add additional bad archive file tests
4 participants