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.

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

cmd/gf: test gen ctrl comments #3575

Merged
merged 5 commits into from
May 27, 2024
Merged

cmd/gf: test gen ctrl comments #3575

merged 5 commits into from
May 27, 2024

Conversation

zcyc
Copy link
Contributor

@zcyc zcyc commented May 11, 2024

Updates #3569
添加测试用例

@oldme-git
Copy link
Member

@gqcn 因为已经使用了标准库 AST 来做语法分析,所以这个测试用例不应该属于框架职责范畴。
Because GoFrame have used AST for parsing,the testing unit case should not be the responsibility of GoFrame.

@zcyc
Copy link
Contributor Author

zcyc commented May 22, 2024

@gqcn 因为已经使用了标准库 AST 来做语法分析,所以这个测试用例不应该属于框架职责范畴。 Because GoFrame have used AST for parsing,the testing unit case should not be the responsibility of GoFrame.

用 AST 能保证解析正确,但不能保证生成正确。测的不是 AST,测的是生成逻辑。
The test is generation, not AST. The AST is correct, but the generation is not necessarily.

@oldme-git
Copy link
Member

@gqcn 因为已经使用了标准库 AST 来做语法分析,所以这个测试用例不应该属于框架职责范畴。 Because GoFrame have used AST for parsing,the testing unit case should not be the responsibility of GoFrame.

用 AST 能保证解析正确,但不能保证生成正确。测的不是 AST,测的是生成逻辑。 The test is generation, not AST. The AST is correct, but the generation is not necessarily.

生成逻辑有其他单测可以确定正确
Generation process have already other testing unit case ensureing the correct.

@wln32
Copy link
Member

wln32 commented May 22, 2024

@gqcn 因为已经使用了标准库 AST 来做语法分析,所以这个测试用例不应该属于框架职责范畴。 Because GoFrame have used AST for parsing,the testing unit case should not be the responsibility of GoFrame.

用 AST 能保证解析正确,但不能保证生成正确。测的不是 AST,测的是生成逻辑。 The test is generation, not AST. The AST is correct, but the generation is not necessarily.

目前2.7.1生成应该没问题吧,之前用的是正则,所以会导致注释掉的也会生成,新版用了ast,准确性没问题,不明白你说的不能保证生成正确说的是哪方面,目前来说api/hello目录下的接口文件是可以做到一个不多,一个不少,但是ctrl目录下不能保证,因为有用户自己写的逻辑,在生成这个目录下的文件的时候,如果是新增的接口,会判断是否存在,如果存在,则不生成,不存在就生成,如果删掉了一个接口,再次执行gf gen ctrl的时候,则不会删除对应的ctrl,好像是这样的逻辑

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@gqcn Because the standard library AST has been used for syntax analysis, this test case should not fall within the scope of the framework's responsibilities. Because GoFrame have used AST for parsing, the testing unit case should not be the responsibility of GoFrame.

Using AST can guarantee that the parsing is correct, but it does not guarantee that the generation is correct. What is tested is not the AST, but the generation logic. The test is generation, not AST. The AST is correct, but the generation is not necessarily.

At present, the generation of 2.7.1 should be no problem. The previous version used regular expressions, so the commented out ones will also be generated. The new version uses ast, and the accuracy is no problem. I don’t understand what you mean by not guaranteeing that the generation is correct. , currently, the interface files in the api/hello directory can be no more than one, but there is no guarantee in the ctrl directory, because there is logic written by the user. When generating files in this directory, if It is a newly added interface. It will be judged whether it exists. If it exists, it will not be generated. If it does not exist, it will be generated. If an interface is deleted and the gf gen ctrl is executed again, the corresponding ctrl will not be deleted. It seems like this. logic

@gqcn
Copy link
Member

gqcn commented May 23, 2024

@oldme-git @wln32 我看了下,这里加一个单例来保证正确性验证也挺好的。我顺便看了下咱们生成ctrl的命令逻辑,似乎有一块没有用ast做替换,是否这里统一使用ast对对后续维护来说更友好一些?
image

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@oldme-git @wln32 I took a look and it would be good to add a singleton here to ensure correctness verification. I took a look at the command logic for generating ctrl, and it seems that there is a part that is not replaced with ast. Is using ast uniformly here more friendly for subsequent maintenance?
image

@gqcn gqcn added the ready to merge Used in PR, which means this PR is reviewed. label May 23, 2024
@oldme-git
Copy link
Member

oldme-git commented May 23, 2024

@oldme-git @wln32 我看了下,这里加一个单例来保证正确性验证也挺好的。我顺便看了下咱们生成ctrl的命令逻辑,似乎有一块没有用ast做替换,是否这里统一使用ast对对后续维护来说更友好一些? image

@gqcn 之前用正则的时候,有很多 issue 都说过关于注释生成有误的问题,如果要加上这块的单测,直接放在 testdata/genctrl/api 下当做基础用例或许更好,而且,ctrl 的其他基础测试用例也不完善,在未来也应该补充进去,可以参考 #3488 中关于 logic 的测试用例。
关于 ctrlAST 重构计划和更多的基础单测用例,是准备在合并 #3488 后进行的。单纯的针对单个 issue 编写不复杂且基础的测试用例,不会为框架带来长久的影响。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@oldme-git @wln32 I took a look and it would be good to add a singleton here to ensure correctness verification. I took a look at the command logic for generating ctrl, and it seems that there is a part that is not replaced with ast. Is using ast uniformly here more friendly for subsequent maintenance? image

When using regular expressions before, there were many issues' about the problem of incorrect annotation generation. If you want to add this single test, it may be better to put it directly under testdata/genctrl/apias a basic use case. Moreover, other basic test cases forctrlare not complete and should be added in the future. You can refer to the test cases for logic in #3488. TheASTrefactoring plan forctrland more basic single test cases are planned to be carried out after merging #3488. Simply writing uncomplicated and basic test cases for a singleissue` will not bring long-term impact to the framework.

@gqcn
Copy link
Member

gqcn commented May 23, 2024

@oldme-git @wln32 我看了下,这里加一个单例来保证正确性验证也挺好的。我顺便看了下咱们生成ctrl的命令逻辑,似乎有一块没有用ast做替换,是否这里统一使用ast对对后续维护来说更友好一些? image

@gqcn 之前用正则的时候,有很多 issue 都说过关于注释生成有误的问题,如果要加上这块的单测,直接放在 testdata/genctrl/api 下当做基础用例或许更好,而且,ctrl 的其他基础测试用例也不完善,在未来也应该补充进去,可以参考 #3488 中关于 logic 的测试用例。 关于 ctrlAST 重构计划和更多的基础单测用例,是准备在合并 #3488 后进行的。单纯的针对单个 issue 编写不复杂且基础的测试用例,不会为框架带来长久的影响。

这个单测可以补充覆盖率,感觉可以先合并进去,后续如果有重构计划或者改进,也可以对这块再调整。#3488 我还在review中,需要花一点时间拿真实的复杂项目测试。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@oldme-git @wln32 I took a look and it would be good to add a singleton here to ensure correctness verification. I took a look at the command logic for generating ctrl, and it seems that there is a part that is not replaced with ast. Is using ast uniformly here more friendly for subsequent maintenance? image

@gqcn When I used regular expressions before, there were many issues' about the problem of incorrect annotation generation. If you want to add this single test, you can put it directly under testdata/genctrl/apias a basic use case. Better. Moreover, other basic test cases ofctrlare not perfect and should be added in the future. You can refer to the test cases about logic in #3488. TheASTrefactoring plan forctrland more basic single test cases are planned to be carried out after merging #3488. Simply writing uncomplicated and basic test cases for a singleissue` will not bring long-term impact to the framework.

This single test can supplement the coverage. I feel that it can be merged into it first. If there are refactoring plans or improvements in the future, this area can also be adjusted. #3488 I am still in review and need to spend some time testing it on real complex projects.

@gqcn gqcn merged commit 9e9e42b into gogf:master May 27, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Used in PR, which means this PR is reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants