Skip to content

Conversation

@mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Sep 30, 2020

I present to you: the first high-level integration test to make sure that executing steps works.

Am I proud of this? Yes and no. Yes, because it gives me confidence that something works and it is a nice integration test. No, because, well, look, I created a little Docker "emulator" in Bash and I'm not sure how I feel about that now.

Edit: also, let me add: it creates a ZIP file in memory, mocks the Sourcegraph backend, actually runs commands in a temp directory and allows us to assert that changes were made to the files!

@mrnugget mrnugget requested a review from a team September 30, 2020 13:26
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

I like it, mocks the necessary parts but keeps us away from actually having to run an extensive campaign, which likely has many failure points.

However, it seems windows hates your test 😬

@mrnugget
Copy link
Contributor Author

However, it seems windows hates your test 😬

Yeah :( I'll try to see what I can do, but ... writing PowerShell? Probably not

@eseliger
Copy link
Member

I guess writing it in go would work, although a little overkill 😬

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

I assume the concern here is running the commands in CI, where we might not have a real Docker available?

I'm surprised there isn't a way to run Docker from within a GitHub Action: we can't be the first to have this issue. Nothing leaps off the page after some Googling, though.

I like this in general, though, and I don't want to let perfect be the enemy of good.

@mrnugget
Copy link
Contributor Author

mrnugget commented Oct 1, 2020

I assume the concern here is running the commands in CI, where we might not have a real Docker available?

Yeah, exactly. And it's also nice that the tests are super fast to run. I'll try to see if I can find out how to run Docker in Docker in GitHub actions, but I was and am afraid that it's not easy to do without changing how we run containers in some way.

@mrnugget mrnugget merged commit c39e1a5 into main Oct 1, 2020
@mrnugget mrnugget deleted the mrnugget/executor-integration-test branch October 1, 2020 04:29
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* Add first high-level integration test for Executor

* Exec bash process in dummydocker

* Bump executor timeout to 5s in integration test

* add docker.exec symlink for windows

* Use OS-specific path separator when setting PATH

* .exe to .bat

* Try cd'ing into bat file directory

* Skip integration test on windows
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