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

Closes test related issues #56

Closed
wants to merge 10 commits into from
Closed

Closes test related issues #56

wants to merge 10 commits into from

Conversation

kyrcha
Copy link
Contributor

@kyrcha kyrcha commented Oct 15, 2019

Closes:

I have also added a new "constructor" in github/downloader.go because the storer is not exposed and I could not get access to the storer for counting the various objects in the recording script.


  • I have updated the CHANGELOG file according to the conventions in keepachangelog.com
  • This PR contains changes that do not require a mention in the CHANGELOG file

@kyrcha kyrcha requested a review from a team October 15, 2019 10:55
@kyrcha kyrcha self-assigned this Oct 15, 2019
github/downloader_test.go Outdated Show resolved Hide resolved
@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 15, 2019

From https://docs.travis-ci.com/user/best-practices-security#steps-travis-ci-takes-to-secure-your-data:

The beta Windows support does not obfuscate secure environment variables leaked into the build log. Please keep reading the next section, on how to avoid leaking secrets to build logs

So I have removed the windows env for possible leaks. Will reintegrate when out of beta.

@smacker
Copy link
Contributor

smacker commented Oct 15, 2019

It's better to create small PRs for each issues than 1 big pr that does everything. That way we can merge fixes and improvements faster.

In general LGTM but I think we need to change constructors to make it accept store.

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 15, 2019

OK will make smaller contributions next time.

In general LGTM but I think we need to change constructors to make it accept store.

OK so I am waiting for this change?

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 15, 2019

I think I did something wrong again. I'll try to fix it.

@smacker
Copy link
Contributor

smacker commented Oct 15, 2019

codecov: -64.22%

OK so I am waiting for this change

nah. it's just a recommendation.

also please use rebase.

@se7entyse7en
Copy link
Contributor

👍 for smaller PRs and rebase.

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 16, 2019

Please do not merge yet. Looking into the low codecov issue

Copy link
Contributor

@se7entyse7en se7entyse7en left a comment

Choose a reason for hiding this comment

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

I only don't know if testutils.Tests is a good name as they're actually not tests themselves. Maybe something like testutils.TestsOracle? But other than that LGTM.

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 16, 2019

Actually I've pinpointed the problem in this part so I will refactor it and update.

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 17, 2019

OK, I think this is ready. The lower coverage comes from extra code added in between. Will rise after closing issue #40. From the next PR, I will work on the git workflow discussed.

@se7entyse7en
Copy link
Contributor

I still don't understand the coverage thing. Shouldn't it increase? Is it caused only by having added the NewMemoryDownloader function?

Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 17, 2019

retry.go has 0% coverage on codcov, while locally it has 40%+. I will further investigate.

@smacker
Copy link
Contributor

smacker commented Oct 17, 2019

@kyrcha do you run locally with a token? Because I see this PR is created from you fork. So maybe it executes only offline tests and retry is covered only in online tests?

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 17, 2019

Yes I run it locally with a token. OK I see your point.

Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
@smacker
Copy link
Contributor

smacker commented Oct 17, 2019

Yes I run it locally with a token. OK I see your point.

most probably we need to open the PR from a branch in metadata-retrieval or just ignore this codecov report and merge as is (as we know it didn't really decrease)

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 17, 2019

OK I am closing this PR and re-submitting it from a branch.

@kyrcha kyrcha closed this Oct 17, 2019
This was referenced Oct 17, 2019
@se7entyse7en
Copy link
Contributor

Oh I didn't realize this was from your fork, I understand now 👍

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.

3 participants