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

Add test case to document env header contains whitespace error as expected behavior. #5081

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MicahKimel
Copy link

- What I did

- How to verify it

  • Running test cases docker buildx bake test looked good to me

- Description for the changelog

Test case documenting using a bad header in .env will throw a error is expected behavior.

- A picture of a cute animal (not mandatory but encouraged)
cat

njucjc and others added 5 commits May 18, 2024 08:59
Signed-off-by: njucjc <njucjc@gmail.com>
Signed-off-by: MicahKimel <micah.kimel@my.wheaton.edu>
Signed-off-by: MicahKimel <micah.kimel@my.wheaton.edu>
Signed-off-by: racequite <quiterace@gmail.com>
Signed-off-by: MicahKimel <micah.kimel@my.wheaton.edu>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: MicahKimel <micah.kimel@my.wheaton.edu>
Signed-off-by: cncal <flycalvin@qq.com>
Signed-off-by: MicahKimel <micah.kimel@my.wheaton.edu>
Copy link
Member

@Benehiko Benehiko 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 for the PR :)
This already looks quite good. Just a few comments from my side.

Comment on lines +100 to +102
if err == nil {
t.Fatalf("Expected an ErrBadKey, got nothing")
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err == nil {
t.Fatalf("Expected an ErrBadKey, got nothing")
}
assert.ErrorIs(t, err, ErrBadKey{"variable '[config 1]' contains whitespaces"})

Copy link
Member

Choose a reason for hiding this comment

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

with this assertion we don't need the other checks below

- A valid CDI specification (JSON or YAML file) for the requested device is
available on the system running the daemon, in one of the configured CDI
specification directories.
- The CDI feature has been enabled in the daemon; see [Enable CDI
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- The CDI feature has been enabled in the daemon; see [Enable CDI
- The CDI feature has been enabled in the daemon. See [Enable CDI

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.33%. Comparing base (28c5652) to head (5fab514).
Report is 101 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5081   +/-   ##
=======================================
  Coverage   61.33%   61.33%           
=======================================
  Files         298      298           
  Lines       20692    20691    -1     
=======================================
  Hits        12691    12691           
+ Misses       7100     7099    -1     
  Partials      901      901           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker run throws error if an env var contains whitespace
7 participants