Skip to content

Conversation

@yevgenypats
Copy link
Contributor

@yevgenypats yevgenypats commented Feb 8, 2023

Add force tests

@yevgenypats yevgenypats changed the title fix: destination testing always use force fix: Destination testing always use force Feb 8, 2023
@github-actions github-actions bot added fix and removed fix labels Feb 8, 2023
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

⏱️ Benchmark results

Comparing with e49f593

  • DefaultConcurrencyDFS-2 resources/s: 10,385 ⬇️ 6.38% decrease vs. e49f593
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,440 ⬆️ 0.40% increase vs. e49f593
  • Glob-2 ns/op: 182.6 ⬆️ 2.90% increase vs. e49f593
  • TablesWithChildrenDFS-2 resources/s: 30,483 ⬇️ 0.98% decrease vs. e49f593
  • TablesWithChildrenRoundRobin-2 resources/s: 27,859 ⬇️ 5.61% decrease vs. e49f593
  • TablesWithRateLimitingDFS-2 resources/s: 28.73 ⬆️ 1.01% increase vs. e49f593
  • TablesWithRateLimitingRoundRobin-2 resources/s: 841.5 ⬆️ 5.89% increase vs. e49f593
  • BufferedScanner-2 ns/op: 9.293 ⬇️ 0.04% decrease vs. e49f593
  • LogReader-2 ns/op: 31.19 ⬆️ 0.10% increase vs. e49f593

@hermanschaaf
Copy link
Member

@yevgenypats Is it wise to only test forced migrations? I think if we need a default to test, it should be non-forced, which has been our default behavior until now

@yevgenypats
Copy link
Contributor Author

@yevgenypats Is it wise to only test forced migrations? I think if we need a default to test, it should be non-forced, which has been our default behavior until now

We don't really use those / use it anywhere apart from the latest postgres destination.

I can add it though those tests in this PR

@yevgenypats yevgenypats changed the title fix: Destination testing always use force fix: Destination testing add force tests Feb 10, 2023
@github-actions github-actions bot added fix and removed fix labels Feb 10, 2023
@github-actions github-actions bot added fix and removed fix labels Feb 10, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Base: 47.37% // Head: 57.02% // Increases project coverage by +9.64% 🎉

Coverage data is based on head (d52d15c) compared to base (e49f593).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #671      +/-   ##
==========================================
+ Coverage   47.37%   57.02%   +9.64%     
==========================================
  Files          70       42      -28     
  Lines        6792     3237    -3555     
==========================================
- Hits         3218     1846    -1372     
+ Misses       3124     1176    -1948     
+ Partials      450      215     -235     
Impacted Files Coverage Δ
schema/column.go
schema/table.go
schema/cidr_array.go
schema/transformer.go
schema/macaddr_array.go
schema/float8.go
schema/meta.go
schema/errors.go
schema/uuid.go
schema/macaddr.go
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yevgenypats
Copy link
Contributor Author

@hermanschaaf added proper tests for force migrations.

I can also add another type of tests where in non-force it returns an error on column type change but prefer to do it in a follow-up PR after we update all destination to this sdk.

@kodiakhq kodiakhq bot merged commit 879f843 into main Feb 12, 2023
@kodiakhq kodiakhq bot deleted the fix/dest_testing branch February 12, 2023 08:45
erezrokah pushed a commit that referenced this pull request Feb 12, 2023
🤖 I have created a release *beep* *boop*
---


##
[1.36.1](v1.36.0...v1.36.1)
(2023-02-12)


### Bug Fixes

* Destination testing add force tests
([#671](#671))
([879f843](879f843))
* Fix source log message, and some debug messages
([#673](#673))
([e49f593](e49f593))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants