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

ci improvements #3688

Merged
merged 12 commits into from
Mar 6, 2020
Merged

ci improvements #3688

merged 12 commits into from
Mar 6, 2020

Conversation

maximelkin
Copy link
Collaborator

No description provided.

@@ -23,7 +23,7 @@ services:

mysql:
image: mysql
command: --default-authentication-plugin=mysql_native_password
command: --default-authentication-plugin=mysql_native_password --sync_binlog=0 --skip-innodb_doublewrite --innodb-flush-log-at-trx-commit=0 --innodb-flush-method=nosync
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add next to this line comment, explaining rationale behind these parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, later

@kibertoad
Copy link
Collaborator

What is still to be done for this PR?

@maximelkin
Copy link
Collaborator Author

Find options for oracle, mssql

@maximelkin
Copy link
Collaborator Author

Not sure about using ramfs - we use too many instances

@kibertoad
Copy link
Collaborator

@maximelkin Another useful improvement would be to migrate from Travis to CircleCI, btw, since Travis has rather frustrating limits on how often jobs are fired, and in general its future doesn't look particularly bright: https://www.reddit.com/r/devops/comments/at3oyq/it_looks_like_ibera_is_gutting_travis_ci_just_a/

Not sure how difficult such migration would be, but I don't think we are using anything particularly fancy out of Travis features.

@kibertoad
Copy link
Collaborator

@ksatirli Can you take a quick expert glance on our existing Travis config (including maximelkin's improvements in this PR) and assess how difficult would it be to do same things with CircleCI?

@maximelkin
Copy link
Collaborator Author

here only database tuning options, so It shouldn't affect travisci -> circleci migration

@kibertoad
Copy link
Collaborator

kibertoad commented Feb 25, 2020

Yeah, I would hope CircleCI also supports passing DB options

@maximelkin
Copy link
Collaborator Author

So, disabling durability on databases not affected test performance (222 seconds as usual)

@kibertoad
Copy link
Collaborator

@maximelkin Could be that Travis does that by default for CI DBs, which makes sense. I would still keep these changes somewhere around, because there is no guarantee CircleCI does the same thing, we would need to measure again; and TBH I wouldn't mind having these commands in config explicitly.

@maximelkin
Copy link
Collaborator Author

@kibertoad no, because we running databases via docker-compose

@kibertoad
Copy link
Collaborator

Ah, I see

@elhigu
Copy link
Member

elhigu commented Feb 26, 2020

Another useful improvement would be to migrate from Travis to CircleCI, btw, since Travis has rather frustrating limits on how often jobs are fired

Has there been problems with travis so far? I don't see why couldn't we have both running at the same time and see fi Circle CI suits us any better. So far I have been pretty happy with tarvis though (after changing to docker setup). Being able to run tests also in windows would be great though.

@kibertoad
Copy link
Collaborator

@elhigu That's a very good point, we should do that!

@maximelkin
Copy link
Collaborator Author

Coverage decreased because of switch from node 8 to node 12, should we temporary reduce it?

@kibertoad
Copy link
Collaborator

@maximelkin Wonder why version switch would affect coverage. Any particular reason why switch was needed prior to (incoming) dropping of Node 8 support?

@kibertoad
Copy link
Collaborator

kibertoad commented Mar 2, 2020

@maximelkin Was it because of DefinitelyTyped/DefinitelyTyped#42275?

update: nvm, unrelated

@briandamaged
Copy link
Collaborator

@kibertoad + @maximelkin : It looks like the oracle tests are only running for node 8. So, that probably explains why the coverage dropped.

@kibertoad
Copy link
Collaborator

@briandamaged That was intentional, though, to speed up execution, I think.

@briandamaged
Copy link
Collaborator

Right. I'm just saying that it would also cause the coverage to drop since the Oracle code is not being tested.

@maximelkin
Copy link
Collaborator Author

Coverage decreased because of switch from node 8 to node 12, should we temporary reduce it?

Wait
this sound really strange, because coverage checked for all node versions, so changes shouldn't affect it

@briandamaged
Copy link
Collaborator

@maximelkin : By default, Travis-CI currently uses the npm run test script. This runs the unit tests, but it does not calculate the coverage for them.

@maximelkin
Copy link
Collaborator Author

Ok, I got this

@maximelkin
Copy link
Collaborator Author

maximelkin commented Mar 2, 2020

TODO: exclude tests dir from coverage

@maximelkin maximelkin changed the title little optimizations for integration tests ci improvements Mar 3, 2020
@maximelkin maximelkin marked this pull request as ready for review March 4, 2020 22:15
@kibertoad
Copy link
Collaborator

@maximelkin What's the end result of this improvement? Did it speed up tests?

@maximelkin
Copy link
Collaborator Author

maximelkin commented Mar 5, 2020

Did it speed up tests

About ~30 seconds for slowest run from batch

@maximelkin
Copy link
Collaborator Author

pulling oracle image is slowest part

@@ -96,6 +96,7 @@
"mysql": "^2.18.1",
"mysql2": "^2.1.0",
"nyc": "^15.0.0",
"oracledb": "^4.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering that testing Oracle is the slowest step, do we want to run it on all Node versions?

Copy link
Member

Choose a reason for hiding this comment

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

We could add an additional fast test that doesn't pull oracle, but I think its better to have more complete tests than fast ones. In either way it will take long to all test setups to finnish

Copy link
Collaborator Author

@maximelkin maximelkin Mar 6, 2020

Choose a reason for hiding this comment

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

Yes, because in other case we can miss some breaking changes from node for oracledb driver code

@kibertoad kibertoad merged commit 2270c11 into knex:master Mar 6, 2020
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.

None yet

4 participants