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

Internal NO_BACKSLASH_ESCAPES support #702

Closed
wants to merge 9 commits into from

Conversation

lauxjpn
Copy link
Contributor

@lauxjpn lauxjpn commented Sep 21, 2019

I included the test (console) project I used to test the functionality before implementing this PR against MySqlConnector and Connector/ODBC.
These need to be integrated into the actual test suite. I just can't get this to run without VS 16.3 (which will be released with .NET Core 3.0 on Monday, i presume).
Generally, all existing tests should work with NO_BACKSLASH_ESCAPES enabled as well.

This addresses #701 and PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#827

…l and no explicit statement terminator, like this one: select 'foo'

Signed-off-by: Laurents Meyer <[email protected]>
…derived classes and MySqlParameter.

Single quotes are now always escaped with an additional single quote instead of a leading backspace.
This addresses everything except triggering/detecting the NO_BACKSLASH_ESCAPES from mysql-net#701 and PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#827

Signed-off-by: Laurents Meyer <[email protected]>
@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch 2 times, most recently from fbb913d to c17ee05 Compare September 21, 2019 19:50
@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 21, 2019

I got the tests running on my machine now. I will therefor add a couple of test cases to the suite and remove the console test project from this PR.

…quote instead of leading backslash).

Signed-off-by: Laurents Meyer <[email protected]>
@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch from c17ee05 to 1618786 Compare September 21, 2019 19:55
else if (state == State.SingleQuotedStringSingleQuote)
{
state = State.Statement;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a general class of error I can push a fix for (before this PR is merged).

Copy link
Member

Choose a reason for hiding this comment

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

5060d9d; will merge to master soon.

@bgrainger
Copy link
Member

The Travis CI tests time out for PRs. I haven't figured out why this is happening (they just seem to run uniformly slower, and I don't know why), but you can ignore them.

Long-term, I plan to migrate to Azure Pipelines. Short-term, I suppose I could just disable Travis CI on PRs, since I know it'll fail.

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 23, 2019

The Travis CI tests time out for PRs. I haven't figured out why this is happening (they just seem to run uniformly slower, and I don't know why), but you can ignore them.

I'll take a quick look at it, if I can figure out why this is happening there.
Also, the promised tests will be up today.

@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch 2 times, most recently from 35abfaf to cdb0c63 Compare September 23, 2019 11:15
Signed-off-by: Laurents Meyer <[email protected]>
@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch from cdb0c63 to 00aea2a Compare September 23, 2019 11:56
@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 23, 2019

The test suite just seems to take to long. Nothing really hangs, the individual tests are just getting executed extremely slow. I have very little experience with Travis CI, so I can't say if this is expected.

The tests could be split into multiple test ENVs and then executed separately/parallel.

But it's probably best to just make the switch to Azure DevOps, as @caleblloyd just did here: PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#823

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 23, 2019

Okay, this came up in a search, regarding Travis CI:

travis-ci/travis-ci#3049 (comment)
travis-ci/travis-cookbooks#888

@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch 3 times, most recently from b195329 to 1813fe9 Compare September 23, 2019 19:57
@bgrainger
Copy link
Member

Those are interesting links, but it seems like that problem has been ongoing for two (or more) years. From PR history, I know that Travis was working in April, but was timing out in May, so the change (for this project) happened in that timeframe.

I need to review what Travis is testing that I haven't replicated on Azure Pipelines and either finish copying it, or decide to leave it untested.

@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch 3 times, most recently from 4e4b4c4 to 73bd33f Compare September 23, 2019 20:24
@lauxjpn lauxjpn force-pushed the no_backslash_escapes branch from 73bd33f to e14a3d2 Compare September 23, 2019 20:32
@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 23, 2019

I ended up disabling most of the tests on Travis CI.
Do the added test scenarios suffice, or should I add some in a particular area?

@bgrainger
Copy link
Member

The new tests look good--thanks! Do you have any preference on how I merge/squash/rebase this?

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 23, 2019

Just squash it.

@bgrainger bgrainger mentioned this pull request Sep 23, 2019
@bgrainger
Copy link
Member

Merged via d550a69.

@bgrainger bgrainger closed this Sep 24, 2019
@lauxjpn lauxjpn deleted the no_backslash_escapes branch September 24, 2019 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants