Skip to content

Add list-files directive - #44

Open
komainu8 wants to merge 2 commits into
groonga:mainfrom
komainu8:add-directive-for-file-exist
Open

Add list-files directive#44
komainu8 wants to merge 2 commits into
groonga:mainfrom
komainu8:add-directive-for-file-exist

Conversation

@komainu8

@komainu8 komainu8 commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This directive is used to test column partitioning.
It is needed to check that files that store partition maps and files that store partitions exist after creation and do not exist after deletion.

Usage:

#@list-files #{db_path}

Results:

 db
 db.0000000
 db.0000100
 db.0000101
 db.0000101.partitions
 db.001
 db.conf
 db.options

@komainu8
Add file-exist directive
53e1051 
This directive is used to test column partitioning.
It is needed to check that files that store partition maps and files
that store partitions exist after creation and do not exist after
deletion.

Usage:

  #@file-exist #{db_path}.0000101.partitions
@abetomo

abetomo commented Oct 9, 2026

Copy link
Copy Markdown
Member

directiveはテストに必要な道具の準備とかをするものだと思うので、 #@file-exist のように直接的にファイルの有無をチェックするのではなくて、 #@list-files #{db_path} みたいな感じでファイルの一覧を表示して、.actual と .expected でチェックするような方法が良さそうに思います。

（ #@file-exist を特定のファイルがない場合はそのテストをスキップ、のような用途で使うのは良いと思います。）

あとはdirectiveを追加しない方法で、ファイルを作成したであろうログのテストや、 object_inspect でテストできるとよさそうです。（groonga側に手を入れないといけないかも）

@komainu8 komainu8 changed the title Add file-exist directive Add list-files directive Oct 13, 2026
@abetomo
abetomo requested a balanced review from Copilot October 13, 2026 10:56
Copilot AI reviewed Oct 13, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Missing-argument handling crashes, and the implementation lists unrelated directory contents.

1 open finding
What changed in this PR

Adds a list-files directive for validating database partition files.

Changes:

  • Expands directive path variables.
  • Lists files when dispatching #@list-files.
File Description
lib/​grntest/​executors/​base-executor.rb Implements and dispatches the new directive.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/grntest/executors/base-executor.rb
Comment on lines +199 to +203
if raw_path.nil?
log_input(line)
log_error("#|e| [file-exist] path is missing")
end
path = Pathname(expand_variables(raw_path))
@kou

kou commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

あとはdirectiveを追加しない方法で、ファイルを作成したであろうログのテストや、 object_inspect でテストできるとよさそうです。（groonga側に手を入れないといけないかも）

こういうアプローチは無理そうなの？

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@komainu8 @abetomo @kou