-
Notifications
You must be signed in to change notification settings - Fork 9
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
Setup test environment inside CI #9
base: master
Are you sure you want to change the base?
Conversation
for the tests
SearchLight
…/2021-06-15-00-40-40-218-2413546544' compatible issue
Manifest.toml |
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 did you remove Manifest.toml from .gitignore?
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.
uups. Added again.
Project.toml
Outdated
@@ -7,7 +7,10 @@ version = "2.0.1" | |||
DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0" | |||
LibPQ = "194296ae-ab2e-5f79-8cd4-7183a0a5a0d1" | |||
Logging = "56ddb016-857b-54e1-b83d-db4d58db5568" | |||
Revise = "295af30f-e4ad-537b-8983-00126c2a3abe" |
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 are all these dependencies added to the project?
a) do we need Revise at all?
b) the test dependencies shouldn't go to the test project?
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.
a) Revise is not needed and is removed now
b) will change this
src/SearchLightPostgreSQL.jl
Outdated
|
||
@info "Created table $table_name" | ||
queryString = string("select table_name from information_schema.tables where table_name = '$table_name'") |
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.
Can you please use uppercase for SQL, like we use everywhere else? Eg: "SELECT table_name FROM ... WHERE ...".
Also, what is the need for string(...)
?
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.
both requests are changed.
) | ||
end | ||
|
||
mutable struct BookWithInterns <: 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.
Can you please update/cleanup the model to the latest API? We no longer use the _table_name, _id, _serializable fields. Just so that it's not confusing for the users looking at the tests to learn.
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.
Can you please remove the .DS_Store files and add them to .gitignore?
.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?
.github/workflows/Test_DB.yml
Outdated
- name: Install SearchLight and SearchLightPostgreSQL | ||
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 packages?
the test depencies from the main .toml file
LibPQ = "194296ae-ab2e-5f79-8cd4-7183a0a5a0d1" | ||
Logging = "56ddb016-857b-54e1-b83d-db4d58db5568" | ||
SearchLight = "340e8cb6-72eb-11e8-37ce-c97ebeb32050" | ||
|
||
[compat] | ||
DataFrames = "1" |
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.
@FrankUrbach You removed the compat entry for DataFrames - pls put it back
|
||
@info "Created table $table_name" | ||
queryString = "SELECT table_name FROM information_schema.tables WHERE table_name = '$table_name'" |
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.
Please use naming consistent with the rest of the code base. In SearchLight (and Julia in general) we use query_string
not queryString
.
Manifest.toml | ||
tests/db/migrations/* | ||
**/.DS_Store* |
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.
Can you also delete the .DS_Store files that were added?
Purpose of the PR:
Setting up a test database inside CI.
Remarks:
Only test of relations are included. Further test scenarios are needed.