Add benchmarker running on Github Actions #612
Conversation
alxndrsn
changed the title
alxndrsn
force-pushed
the
benchmarker-github-actions-master
branch
5 times, most recently
from
6a23c82
to
334be49
Compare
matthew-white
requested review from
sadiqkhoja
and removed request for
matthew-white
There was a problem hiding this comment.
I think @sadiqkhoja is going to take a look at the benchmarker, but I was wondering whether it'd be helpful to move the changes to lib/bin/check-file-headers.js to a separate PR. I'd be happy to take a look at those changes!
Also, I see the new file benchmarker/scripts/ci-initdb.js, but could lib/bin/create-docker-databases.js be used instead?
alxndrsn
force-pushed
the
benchmarker-github-actions-master
branch
from
334be49
to
5256cfc
Compare
.circleci/config.yml
Outdated
| @@ -11,7 +11,7 @@ jobs: | |||
|
|
|||
| steps: | |||
| - checkout | |||
| - run: node lib/bin/check-file-headers.js | |||
| - run: git ls-files | node lib/bin/check-file-headers.js | |||
There was a problem hiding this comment.
It would be helpful to have this script run as part of make test as well
There was a problem hiding this comment.
Or a bit annoying when developing locally?
lib/bin/check-file-headers.js
Outdated
|
|
||
| for (const entryPath of files) { | ||
| const contents = fs.readFileSync(entryPath).toString(); | ||
| const withConsistentYear = contents.replace(/(Copyright 20)\d\d/, '$117'); |
There was a problem hiding this comment.
can we ensure that year is also correct?
There was a problem hiding this comment.
I've thought a little about this, and I think that might be tricky. We could try checking that all new files specify the current year in their file header — that'd work in most cases. But files are sometimes moved, which makes things hard: a file that was moved may look like a new file, yet specify a previous year in its file header. Things also get challenging around the year boundary, because a single PR may have commits from both years.
So I don't think CircleCI should fail if the year in the file header isn't the current year. If there's an easy way for it just to provide a warning in that case, I could see that being helpful in a follow-up PR — not sure what the possibilities there are.
| timeout-minutes: 10 | ||
| run: ./benchmarker/scripts/ci-benchmark | ||
| - name: Backend Logs | ||
| if: always() |
There was a problem hiding this comment.
do we need if: always()?
benchmarker/index.js
Outdated
| Content-Disposition: form-data; name="xml_submission_file"; filename="submission.xml"\r | ||
| Content-Type: application/xml\r | ||
| \r | ||
| <data id="250_questions"> |
There was a problem hiding this comment.
I don't see any advantage of having random values here, only unique instanceID should suffice
Then we can move this to a separate xml file
There was a problem hiding this comment.
Non-random values will likely affect behaviour and response size for zipped streams.
There was a problem hiding this comment.
I think this could be moved to a separate file regardless, if that would be helpful.
|
It looks like there's a conflict now that #619 is merged. Might be worth rebasing? |
alxndrsn
force-pushed
the
benchmarker-github-actions-master
branch
2 times, most recently
from
0cb2529
to
be5723e
Compare
alxndrsn
force-pushed
the
benchmarker-github-actions-master
branch
from
8e09b6c
to
548eebc
Compare
Example runs can be seen at https://github.com/alxndrsn/odk-central-backend/actions