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

feat: add support for sequlize migration options #653

Merged

Conversation

tim-yao
Copy link
Contributor

@tim-yao tim-yao commented Jul 16, 2021

For suggestion from #652

@google-cla
Copy link

google-cla bot commented Jul 16, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@tim-yao
Copy link
Contributor Author

tim-yao commented Jul 16, 2021

@googlebot I signed it!

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks very much for the contribution @tim-yao! :D

impl LGTM, just a test request

packages/server/test/mysql-server-migration.test.js Outdated Show resolved Hide resolved
it('should use the renamed table', async () => {
const sequelize = new Sequelize(process.env.MYSQL_DB_URL);
const tables = await sequelize.query('show tables');
await sequelize.close();
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you put this line in a finally block? if our query fails we still want the server connection to close so test process will exit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @patrickhulce! Updated.

@patrickhulce
Copy link
Collaborator

also fyi have some lint errors to fix :) https://github.com/GoogleChrome/lighthouse-ci/pull/653/checks?check_run_id=3094377453#step:9:33

@patrickhulce
Copy link
Collaborator

@tim-yao I think we're running into issues with the test being run in the same set as the original mysql test because they're using the same DB. let's just go ahead and change the existing test to use the custom migration table name and add our assertion there rather than re-run the mysql set. WDYT?

@patrickhulce
Copy link
Collaborator

That or switch to sqlite database in a new file name that's a separate DB.

@tim-yao
Copy link
Contributor Author

tim-yao commented Jul 19, 2021

@tim-yao I think we're running into issues with the test being run in the same set as the original mysql test because they're using the same DB. let's just go ahead and change the existing test to use the custom migration table name and add our assertion there rather than re-run the mysql set. WDYT?

That make sense to me. Will do it.

@patrickhulce any idea? I get these errors in test with the new migration configs.I can see them in the CI as well.
Screen Shot 2021-07-20 at 9 52 26 am

@patrickhulce
Copy link
Collaborator

any idea? I get these errors in test with the new migration configs.I can see them in the CI as well.

You haven't pushed the latest version with the suggestion so I'm not sure. Those errors are expected with the version of the code that has been pushed here (the databases are being reused between 2 tests with unexpected state).

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.

2 participants