-
Notifications
You must be signed in to change notification settings - Fork 7
Setting up test environment #6
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
Conversation
.github/workflows/Test_DB.yml
Outdated
@@ -0,0 +1,40 @@ | |||
name: PostgreSQLAdapter test |
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.
Let's name this "MySQLAdapter test" please
.github/workflows/Test_DB.yml
Outdated
- uses: actions/checkout@v2 | ||
- uses: julia-actions/setup-julia@latest | ||
with: | ||
version: '1.6.4' |
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.
Bump to 1.7?
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.
changed
.github/workflows/Test_DB.yml
Outdated
- name: Install SearchLight and SearchLightMySQL | ||
run: | | ||
pwd | ||
julia -e 'using Pkg; Pkg.add(url="https://github.com/FrankUrbach/SearchLight.jl.git")' |
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.
Link to official package?
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.
changed
@@ -0,0 +1,79 @@ | |||
[deps] |
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.
What happened to this file? Why so many dependencies?
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.
file is deleted
test/models.jl
Outdated
end | ||
|
||
mutable struct BookWithInterns <: AbstractModel | ||
### INTERNALS |
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.
Internal fields should no longer be needed with the latest API - can you please remove?
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.
changed
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.
The internal (the ones starting with _
) are still here though...
Project.toml
Outdated
Logging = "56ddb016-857b-54e1-b83d-db4d58db5568" | ||
MySQL = "39abe10b-433b-5dbd-92d4-e302a9df00cd" | ||
Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f" | ||
SafeTestsets = "1bc83da4-3b8d-516f-aca4-4fe02f6d838f" |
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.
The test dependencies must not be in the library's Project (they are not dependencies of SearchLightMySQL itself). The /test
folder is treated as a separate project and that's where we add the test dependencies. Please see for example how it's done for Genie.jl.
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.
In the Postgres adapter you've said that you want have the Project.toml not in the test folder. And the test task in CI needs the Pkg and SafeTestsets to work properly.
If we work out the tests as a separat project inside the test folder I can change this back. Should I?
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.
1/ I'd be surprised to have said that. Can you double check/show me?
2/ CI needs them to be in the package's Project file?
3/ Yes, that is the recommended structure for testing Julia packages.
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.
1/ See GenieFramework/SearchLightPostgreSQL.jl#9 (comment)
As you have commented above I will change the structure of the test folder back to the structure needed for a project and will commit it if it's done.
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.
Thanks - so re 1/ it was a question "the test dependencies shouldn't go to the test project?". The answer is "yes, they should". I should've been more clear.
export Callback | ||
export seed, fields_to_store | ||
|
||
mutable struct Book <: AbstractModel |
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.
This could use the newer syntax based on @kwdef
- but the this one should work equally well.
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.
The @kwdef phrase is only needed if you want to specify default values by creating instances of a struct. If this isn't needed because you set the values of the fields later you can use it in the form used here. But I know what you mean.
Looks really good @FrankUrbach thank you! |
Purpose of the PR:
Setting up a test environment inside CI for testing purposes.
Remarks:
Only the relation tests are imported in the adapter. Further tests are needed.