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

Add more downloader tests by recording and replaying #29

Merged
merged 14 commits into from
Oct 14, 2019

Conversation

kyrcha
Copy link
Contributor

@kyrcha kyrcha commented Oct 9, 2019

This PR includes:

  • examples/cmd/testing/recordingscript.go: a script that records requests (the query string) and responses (the data) of the downloader and stores them in a gob file for later replaying. I am not sure if this is the correct place to put it, but I wanted it to run from CLI that is why I added it to the main package.
  • github/downloader_test.go: which augments the tests with offline tests that do not need the GH token and also enable pagination to take place.
  • 2 .gob files and their json oracles that replay the crawling of the src-d organization and the src-d/gitbase repo. More tests like that can be added.

Signed-off-by: Kyriakos Chatzidimitriou [email protected]

@kyrcha kyrcha added the enhancement New feature or request label Oct 9, 2019
@kyrcha kyrcha self-assigned this Oct 9, 2019
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 9, 2019

Thanks for the comments. I will resolve them ASAP.

github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
testutils/memory.go Outdated Show resolved Hide resolved
testutils/memory.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
…s, add integration tests with DB

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

smacker commented Oct 11, 2019

But making them lossier (i.e., number of prs > 2 vs. number of prs == 2) is a good idea.

that is exactly what I had in mind.

change big gob files

good point. thanks! Even I don't think 5mb is actually big but anyway, in my opinion, it would be a good idea to compress the fixtures. I don't know how compressible gob files are. But we can always change the underlying format to something else like msgpack if gob doesn't fit us.

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 11, 2019

OK I'll check compression, and if it works (well enough) I'll let you know

@se7entyse7en
Copy link
Contributor

Another concern I have is that when later we need to change big gob files (5MB for example) containing recordings whether this will impact the performance of git due to add/rm large files. WDYT?

Another idea could be to track them separately in another repo, so before the tests, we just download the version we need. But as @smacker said I don't think that 5MB would cause many issues.
The downside of this approach is that we would require having an internet connection also for recorded data.

// 7. Offline (recorded) download without token for src-d org and DB
// 8. Offline (recorded) download without token for src-d/gitbase repo and DB

type DownloaderTestSuite struct {
Copy link
Contributor

@se7entyse7en se7entyse7en Oct 11, 2019

Choose a reason for hiding this comment

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

Maybe I'd have done something like:

type DownloaderTestSuite struct {
	suite.Suite
	db         *sql.DB
	downloader *Downloader
	jsonTests  testutils.Tests
}

type DownloaderOnlineMemoryTestSuite struct {
	DownloaderTestSuite
}

type DownloaderOfflineMemoryTestSuite struct {
	DownloaderTestSuite
}

type DownloaderOnlineDBTestSuite struct {
	DownloaderTestSuite
}

type DownloaderOfflineDBTestSuite struct {
	DownloaderTestSuite
}

with 2 tests each, one for repo and one for org. Most probably many things for setup could be unified in DownloaderSuite instead of repeating them inside each test. But just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is nicer.

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

kyrcha commented Oct 11, 2019

Another concern I have is that when later we need to change big gob files (5MB for example) containing recordings whether this will impact the performance of git due to add/rm large files. WDYT?

Another idea could be to track them separately in another repo, so before the tests, we just download the version we need. But as @smacker said I don't think that 5MB would cause many issues.
The downside of this approach is that we would require having an internet connection also for recorded data.

With gzip they are now 80% smaller so perhaps we could keep it like this and if they keep changing polluting the git history we can move them to another repo.

@lwsanty
Copy link
Contributor

lwsanty commented Oct 11, 2019

@se7entyse7en @kyrcha
moving these files to a separate repo concerns me more than having them in this repo as test data.

If we wanted to use this data for regression scope one day(this day's gonna be quite soon)
it's better to have test data "attached" to the version, like in gitbase project

In the other case we should keep the relations between current repo's version and test data repo's version and it's pretty much painful

am I missing something?

Comment on lines +24 to 26
"github.com/src-d/metadata-retrieval/database"
"github.com/src-d/metadata-retrieval/github/store"
"github.com/src-d/metadata-retrieval/testutils"
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for pointing at this each time, but these are internal imports :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry. Visual studio code is rearranging stuff all the time and then I forget to manually rearrange them. I should add some pre-commit hook to handle this stuff. Do you have something to propose?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do it manually tbh, but probably there should be some linter for this purpose

@smacker
Copy link
Contributor

smacker commented Oct 11, 2019

The current size of gob files is 800kb. One JPEG picture may have a size of a few mb. I don't think we should care about this 800kb at all.

github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
github/downloader_test.go Outdated Show resolved Hide resolved
@se7entyse7en
Copy link
Contributor

@lwsanty

In the other case we should keep the relations between current repo's version and test data repo's version and it's pretty much painful

True, even if I don't know how much painful would it be, but you guys know surely better than me how to handle these things. Also, in this case, it's not a real problem to think about. 👍

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

lwsanty commented Oct 14, 2019

@kyrcha just a kind reminder that some minor merge conflicts left

@smacker
Copy link
Contributor

smacker commented Oct 14, 2019

Yeah. Let's solve the conflicts and merge!

@kyrcha
Copy link
Contributor Author

kyrcha commented Oct 14, 2019

Tests are failing due to newer methods. Will fix the issues and ping you back

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

kyrcha commented Oct 14, 2019

  • Fix some old method signatures
  • Update recordings to incorporate different queries done now
  • Update oracles
    @src-d/applications

@smacker smacker merged commit c581ad6 into src-d:master Oct 14, 2019
@smacker
Copy link
Contributor

smacker commented Oct 14, 2019

Thanks! But better use rebase next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants