Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Salesforce migration #629
Salesforce migration #629
Changes from 9 commits
3615647
66d9063
7951428
d08d07f
8b99f6a
773797d
7fc3688
ee468c9
4046167
bf79a66
a3fea0e
f976ecd
f5deb9a
ddee24f
0e07e8d
3aaad06
d0f0416
63913e0
a8fa335
b408db7
ab1d1af
de73e4c
61bf161
47e177e
3e2ef08
97e6dbb
5625cee
4215dea
b6e311e
c8bdf4e
5adfbe2
570b579
4415009
990775a
03d07c4
7885ba7
ea0fa2a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You should not use a function you're about to test; also the teardown should be cleaning up resources, not creating them
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.
✅ Improved cleaning after tests
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 second point remains, I think it may be confusing because you're mixing 2 things together in these fixtures, you're creating a test df but also randomly upserting records. It's actually not easy to understand the tests since you already upsert records in the fixture, I don't think this should be happening?
Perhaps you meant to insert some records before running
upsert()
test in order to validate that they are correctly updated in theSalesforce.upsert()
method (to validate the "update" part ofupsert()
's functionality)? But in that case, I don't think this needs to be in a fixture, you can do this (note also there is a cleanup at the end, pls also add it - records should be created and removed in each 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.
To be discussed
I have changed these structures, now in fixtures I create a test record in the database and when the test is finished it is deleted
✅ Chenged testing structure in upsert tests
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.
Is it possible to add some limit to the query to not download the whole table?
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.
✅ Renamed test and fixed typos