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

Adding Tests + CI #11

Open
bhuvy2 opened this issue Mar 21, 2019 · 7 comments
Open

Adding Tests + CI #11

bhuvy2 opened this issue Mar 21, 2019 · 7 comments

Comments

@bhuvy2
Copy link
Contributor

bhuvy2 commented Mar 21, 2019

The CI can be mostly copied over from broadway-api, but it'd be nice to have some assurance checks on each commit.

@ayushr2
Copy link
Collaborator

ayushr2 commented Mar 22, 2019

I think this issue needs to be shifted to Chain Link. Broadway grader just communicates with the API and passes on the job to chainlink to run. @nmagerko wrote some tests for chain link which tests what we want to test.

What would we want to test on the broadway-grader side of things?

@bhuvy2
Copy link
Contributor Author

bhuvy2 commented Mar 22, 2019

There is also a discussion of whether we want to merge chain link. Assuming we want to keep the different, we should just sanity test heartbeating and whether chainlink calls the right funcs

@ayushr2
Copy link
Collaborator

ayushr2 commented Mar 23, 2019

Yep, I think we should keep them separate. Chainlink is meant to be a public python library for running a chain of containers. This project is more Broadway specific.

@bhuvy2
Copy link
Contributor Author

bhuvy2 commented Mar 23, 2019

I don't want to get into the whole debate of what warrants a package
(I strongly side on functions or few functions don't warrant packages re: the left-pad dabacle, and chain-link in spirit is a different functionality, but is not a collection of functions or otherwise.

Either way, mocking should be added.

@nmagerko
Copy link
Collaborator

nmagerko commented Mar 23, 2019

For testing, I think this project might need some refactoring first (this project can be tested in a way that is separate from how chainlink tests). We should at least be able to test that a job is submitted to some black box (e.g. chainlink mocked) to run, and that the results we get back from the broadway-grader routine that does that makes sense.

@nmagerko
Copy link
Collaborator

nmagerko commented Mar 23, 2019

The actual execution of running containers in sequence should be tested more throughly, but that is not the responsibility of this project. I'd say that needs to be a chainlink issue.

@zhengyao-lin zhengyao-lin transferred this issue from illinois-cs241/broadway-grader Sep 29, 2019
@ayushr2
Copy link
Collaborator

ayushr2 commented Jan 28, 2020

As of now,

  • We have added integration tests which run the api and grader together.
  • Nick has refactored this project already.
  • Chainlink has some tests.

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

No branches or pull requests

3 participants