Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

KafkaJS v2#4427

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
edoardo-bluframe wants to merge2 commits intoflow-typed:main
base:main
Choose a base branch
Loading
frombluframe:kafkajs
Open

Conversation

@edoardo-bluframe
Copy link
Contributor

@edoardo-bluframeedoardo-bluframe commentedMar 27, 2023
edited
Loading

Links to documentation:https://www.npmjs.com/package/kafkajs
Link to GitHub or NPM:https://github.com/tulios/kafkajs
Type of contribution: addition

/definitions/npm/sequelize_*/**/*@jedwards1211

/definitions/npm/kafkajs_*/**/*@edoardo-bluframe
/definitions/npm/react-helmet_v6*/**/*@edoardo-bluframe
Copy link
Member

Choose a reason for hiding this comment

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

Check outthis to add code owners in the respective definition instead for the service to do it's thing

});

it("uses log levels",()=>{
constlogLevels=[
Copy link
Member

Choose a reason for hiding this comment

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

This might better test the expectation, your current code block doesn't actually test anything at all

Suggested change
constlogLevels=[
constlogLevels:number[]=[

it("creates Consumer instance",()=>{
constkafka=newKafka({
clientId:"my-app",
brokers:["kafka1:9092","kafka2:9092"],
Copy link
Member

Choose a reason for hiding this comment

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

Suggestions for the constructor, you might want to exercise the options,

  • can it accept all the expected options
  • what if you pass in the invalid values for the option
  • what if you pass in an option that doesn't exist?

Then once constructed withkafka returned to you, does it has the functions and properties you expect but at the same time, does it do things you don't expect?

Copy link
Member

Choose a reason for hiding this comment

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

Definition itself looks good, I can't fault it's structure.

Regarding tests though, I see that you've approached this with use cases in mind which is good, shows that it works as you expect. But the core thing about tests is that you're trying to exercise the definition to prove 2 things, what you've written is actually valid and if someone were to change your definition it would break expectedly.

You'll need some// $FlowExpectedError samples in some places, the fact that you don't have any begs the question, how do we know that your types are actually strict and that they're not justany and you can actually call any function against it?Here is an example of what that might look like, though you don't need to go to the extreme levels of testing everything as I understand it can be tedious, but enough testing so we don't assume a type can be used in the wrong way.

If a nutshell, if you have a block that provesit does this you should also have another block similar that statesit shouldn't be able to do this

@gantoinegantoine added the changes requestedChanges have been requested by a reviewer labelMay 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@BrianzchenBrianzchenBrianzchen left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

changes requestedChanges have been requested by a reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@edoardo-bluframe@Brianzchen@gantoine

[8]ページ先頭

©2009-2025 Movatter.jp