-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix Functional Tests #13585
Fix Functional Tests #13585
Conversation
Related to #13480 |
global.log("version: 3"); | ||
|
||
// "dotnet" command arguments. | ||
let runArgs = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this new code? It's displaying error messages
- name: Allows NodeJs to find the dotnet command | ||
run: | | ||
shell: pwsh | ||
$Env:Path += [IO.Path]::PathSeparator + "/usr/share/dotnet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried something similar but it didn't work for me, if that's working then it's great. That's weird though that it broke at some point, maybe they have an issue with their base images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be part of the js script? Or could the js script try to perfix dotnet with this path (which is what I had tried I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will see
Now it fails because it doesn't find powershell ;)
OCI runtime exec failed: exec failed: unable to start container process: exec: "pwsh": executable file not found in $PATH: unknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be part of the js script?
I will check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing only with SQLServer, not sure why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because for testing I only updated the SQLServer test
So it means that we would also need to update the PATH for powershell or use directly the available command line in the containers we have initialized.
For info with mysql (or mariadb) some of our inner indexes keys are too long with the So to be able to use the default collation we would need to reduce some of our inner indexes keys. Finally I could create a step to connect to the mariadb service which runs in its own container, so I needed to install Doing this all Functional Tests work, also for MySql. |
What size should the indexes be? Where should the change be done? (reducing the size of the index name) |
In a given inner index the total of the included columns sizes should not be greater than 3072 bytes. So with the I don't remember all the failing inner indexes but easy to retrieve. For example the
|
Just to say that currently all tests pass with the collation set to But if you want I can try to reduce the size of the failing inner indexes. |
We should change the name of the index to work with the defaults. At least that will make new sites work. And existing sites that don't change the collation and upgrade their mysql version will then have to change their existing ones. There is a limit defined in yessql AFAIR, Dean worked on that, we may need to update it there too. |
Okay, all tests pass without having to change the MySql collation. I jus needed to change the size of the Workflow indexes by specifying that the Edited: For info the problem only happens when you define an inner |
Fixes #13635
Fixes #13480
I needed to add the
actions/setup-dotnet@v3
step to each test. Now it works for all databases except for MySql for which we get "max key length" errors that seem to be erroneous.With MySql we get
Specified key was too long; max key length is 3072 bytes
errors when creating inner indexes in migrations, which seems erroneous, maybe it can be fixed by configuring a different char set and collation, but for now I just commented out the MySql test.