Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(binding/nodejs): upgrade test behavior and infra #3297

Merged
merged 37 commits into from
Nov 1, 2023
Merged

feat(binding/nodejs): upgrade test behavior and infra #3297

merged 37 commits into from
Nov 1, 2023

Conversation

eryue0220
Copy link
Contributor

Related: #3066

@Xuanwo Xuanwo changed the title test(binding/nodejs): upgrade test behavior and infra feat(binding/nodejs): upgrade test behavior and infra Oct 16, 2023
Xuanwo
Xuanwo reviewed Oct 16, 2023
View reviewed changes
bindings/nodejs/index.js Outdated Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
@eryue0220
fix: cr & ci problems
4022996
@eryue0220
Merge branch 'feat/nodejes-test-suit' of github.com:eryue0220/incubat…
9f711eb 
…or-opendal into feat/nodejes-test-suit

* 'feat/nodejes-test-suit' of github.com:eryue0220/incubator-opendal: (23 commits)
  docs: Add docs in website for sqlite/mysql/postgresql services (#3290)
  chore(bindings/java): align mapping POJO pattern (#3289)
  refactor(bindings/java): explict error handling (#3288)
  feat(layer/prometheus): Support custom metric bucket for Histogram (#3275)
  feat(bindings/java): convey backtrace on exception (#3286)
  refactor(oay): import dav-server-opendalfs (#3285)
  feat(binding/java): make Metadata a POJO (#3277)
  feat(services/rocksdb): change blocking_x in async_x call to tokio::task::blocking_spawn (#3279)
  feat(services/sled): change blocking_x in async_x call to tokio::task::blocking_spawn (#3280)
  feat(binding/c): Add blocking_reader for C binding (#3259)
  Add blocking layer for C bindings (#3278)
  feat(services/redb): change blocking_x in async_x call to tokio::task::blocking_spawn (#3276)
  refactor(bindings/C): Implement error with error message (#3250)
  test(binding/java): grouping tests with tag (#3271)
  reafctor: Polish error process code for mysql/postgresql services (#3269)
  feat(service/d1): Support d1 for opendal (#3248)
  feat: Add edge test cases for OpenDAL Core (#3274)
  ci: Passing GITHUB_TOKEN to avoid rate limit (#3272)
  ci: add a new test case for the disk is full (#3079)
  ci: Only fetch origin while in pull request (#3268)
  ...
Xuanwo
Xuanwo reviewed Oct 16, 2023
View reviewed changes
bindings/nodejs/package.json Outdated Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
Xuanwo
Xuanwo reviewed Oct 17, 2023
View reviewed changes
bindings/nodejs/generated.js Show resolved Hide resolved
bindings/nodejs/tests/test_service.test.js Outdated Show resolved Hide resolved
This was referenced Oct 17, 2023
Merged
Closed
@eryue0220
Copy link
Contributor Author

Currently, the scheme and config will be loaded from the .env configuration.

Xuanwo
Xuanwo reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just found that I didn't submit my review.

bindings/nodejs/tests/suites/index.mjs Show resolved Hide resolved
@eryue0220 eryue0220 marked this pull request as draft November 1, 2023 01:47
@eryue0220 eryue0220 marked this pull request as ready for review November 1, 2023 02:19
@Xuanwo
Copy link
Member

Xuanwo commented Nov 1, 2023

Hi, @eryue0220, this PR is almost set. Can I directly push some additional changes to this PR to align it with the modifications we've made for the main branch?

@eryue0220
Copy link
Contributor Author

most set. Can I directly push some additional changes to this PR to align it with the modifications we've made for the main bra

Yeah, for sure

@Xuanwo
Merge remote-tracking branch 'origin/main' into feat/nodejes-test-suit
82606ea
@Xuanwo
Generate plan for nodejs
9ab39d3 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix typo
fd28c5f 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix working directory
6f26685 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix config loading
d59ea54 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
polish
28690c7 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix format
8354fb7 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
FIx dotenv
cee014a 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix tests
82e9f32 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Member

Xuanwo commented Nov 1, 2023

This feature still have a lot work to do:

  • implement capability
  • implement OPENDAL_DISABLE_RAMDOM_ROOT

But this PR is large enough that I removed not passing test cases to make it mergable first. We can improve it in the coming PRs.

@Xuanwo
Copy link
Member

Xuanwo commented Nov 1, 2023

Apologies for the length of this PR, it has taken @eryue0220 more time than anticipated.

I learnt some lessons from this PR:

  • Don't start work too soon: It's better to discuss about the details first especially for complex work and new contributors like this one.
  • Don't push work too large: It's better to split work in different PRs instead of a huge one.

I believe that we could make the exprience much better if we:

  • Implement the test framework first
  • Integrate with the simplest test case to make sure it works
  • Expand to all services
  • Remove old tests

Regardless, thank you @eryue0220 for your patience and hard work! I promise that we will make the exprience better ❤️

Xuanwo
Xuanwo approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THank you @eryue0220!

@Xuanwo Xuanwo merged commit 8618b57 into apache:main Nov 1, 2023
104 of 105 checks passed
@eryue0220
Copy link
Contributor Author

eryue0220 commented Nov 1, 2023 •
edited

THank you @eryue0220!

Welcome. I appreciated the opendal WG and your effort. I also really learned more from this contribution experience. Finally, this is my first Apache PR, achieve.

@silver-ymz silver-ymz mentioned this pull request Nov 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suyanhanx suyanhanx suyanhanx left review comments

@Xuanwo Xuanwo Xuanwo approved these changes

@PsiACE PsiACE Awaiting requested review from PsiACE PsiACE is a code owner

Assignees
No one assigned
Labels
releases-note/feat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eryue0220 @Xuanwo @suyanhanx