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

test (e2e) : move manpage checks to a separate scenario (#4608) #4611

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rohanKanojia
Copy link
Contributor

@rohanKanojia rohanKanojia commented Feb 11, 2025

Description

Fixes: #4608

Relates to: #4586

In #4586 , I had added step to check man -P cat crc to verify whether man pages are correctly generated after executing crc setup in basic scenario

  • Currently, man pages are only generated for Linux/MacOS. I had added this in basic scenario which caused failure on Windows.
  • On MacOS, I'm seeing an issue that if for some reason manpath is not updated correctly; man command fails to list manpages for CRC. I see that there is an already existing entry for some crc named man page (see man pages didn't cleaned after crc cleanup #4608 (comment))

Update basic.feature to remove manpage checks and add a new scenario manpages.feature to verify manpage generation and cleanup. This new scenario would only be enabled for @darwin and @linux environments.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Proposed changes

These changes update how we verify manpage generation:

  • Update manpages step in basic scenario to skip execution for windows
  • Instead of relying of man command output, only verify whether we've generated the man pages files correctly in the specified directory.

Testing

I've run manpages.feature on MacOS, Linux machine to verify that these changes don't break anything.

Contribution Checklist

  • I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Which platform have you tested the code changes on?
    • Linux
    • Windows
    • MacOS

Copy link

openshift-ci bot commented Feb 11, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

openshift-ci bot commented Feb 11, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gbraad for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rohanKanojia rohanKanojia marked this pull request as ready for review February 11, 2025 16:43
@rohanKanojia rohanKanojia changed the title test (e2e) : update basic scenario to skip manpages check on windows (#4608) test (e2e) : move manpage checks to a separate scenario (#4608) Feb 12, 2025
…rc-org#4608)

+ Update manpages step in basic scenario to skip execution for windows
+ Instead of relying of man command output, only verify whether we've generated the man pages files correctly in the directory.

Signed-off-by: Rohan Kumar <[email protected]>
manPageDir := filepath.Join(userHomeDir, ".local", "share", "man", "man1")
for _, manPage := range expectedManFileList {
manFile := filepath.Join(manPageDir, manPage)
manCommand := fmt.Sprintf("man -P cat %s", manFile)
Copy link
Member

Choose a reason for hiding this comment

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

should we be checking man -P cat crc instead of using the manpage filepath?

i think the generation of the files is already covered by the unit tests and here we can check for if crc setup also updated the required env variables for having the manpages directly accessible by man

Copy link
Contributor Author

@rohanKanojia rohanKanojia Feb 14, 2025

Choose a reason for hiding this comment

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

I added this instead of directly checking the command name because man wasn't discovering the ~/.local/share/man/man1 directory on the QE lab instance.

There was also a problem with MacOS as there was a package named crc that had a manpage (see #4608 (comment))

ture:16
    Then executing "man -P cat crc" fails # features/manpages.feature:19
      Error: command 'man -P cat crc', expected to fail, exited with exit code: 0
Command stdout: crc(n)                                                                  crc(n)
______________________________________________________________________________
NAME
       crc - Message digest "crc"
SYNOPSIS
       package require Tcl  ?8.2?
       package require Trf  ?2.1.4?
       crc ?options...? ?data?
______________________________________________________________________________
DESCRIPTION
       The command crc is one of several message digests provided by the
       package trf. See trf-intro for an overview of the whole package.
       crc ?options...? ?data?
              The options listed below are understood by the digest if and
              only if the digest is attached to a channel.  See section
              IMMEDIATE versus ATTACHED for an explanation of the term
              attached.
              -mode absorb|write|transparent
                     This option has to be present. The specified argument
                     determines the behaviour of the digest in attached mode.
                     Beyond the argument values listed above all unique
                     abbreviations are recognized too. Their meaning is
                     explained below:
                     absorb All data written to the channel is used to
                            calculate the value of the message digest and then
                            passed unchanged to the next level in the stack of
                            transformations for the channel the digest is
                            attached to.  When the channel is closed the
                            completed digest is written out too, essentially
                            attaching the vlaue of the diggest after the
                            information actually written to the channel.
                            When reading from the channel a value for the
                            digest is computed too, and after closing of the
                            channel compared to the digest which was attached,
                            i.e. came behind the actual data.  The option
                            -matchflag has to be specified so that the digest
                            knows where to store the result of said
                            comparison. This result is a string and either
                            "ok", or "failed".
                     write  All data read from or written to the channel the
                            digest is attached to is ignored and thrown away.
                            Only a value for the digest of the data is
                            computed.  When the channel is closed the computed
                            values are stored as ordered through the options
                            -write-destination, -write-type, -read-
                            destination, and -read-type.
                     transparent
                            This mode is a mixture of both absorb and write
                            modes. As for absorb all data, read or written,
                            passes through the digest unchanged. The generated
                            values for the digest however are handled in the
                            same way as for write.
              -matchflag varname
                     This option can be used if and only if the option "-mode
                     absorb" is present. In that situation the argument is the
                     name of a global or namespaced variable. The digest will
                     write the result of comparing two digest values into this
                     variable. The option will be ignored if the channel is
                     write-only, because in that case there will be no
                     comparison of digest values.
              -write-type variable|channel
                     This option can be used for digests in mode write or
                     transparent. Beyond the values listed above all their
                     unique abbreviations are also allowed as argument values.
                     The option determines the type of the argument to option
                     -write-destination. It defaults to variable.
              -read-type variable|channel
                     Like option -write-type, but for option -read-
                     destination.
              -write-destination data
                     This option can be used for digests in mode write or
                     transparent.  The value data is either the name of a
                     global (or namespaced) variable or the handle of a
                     writable channel, dependent on the value of option
                     -write-type. The message digest computed for data written
                     to the attached channel is written into it after the
                     attached channel was closed.  The option is ignored if
                     the channel is read-only.
                     Note that using a variable may yield incorrect results
                     under tcl 7.6, due to embedded \0's.
              -read-destination data
                     This option can be used for digests in mode write or
                     transparent.  The value data is either the name of a
                     global (or namespaced) variable or the handle of a
                     writable channel, dependent on the value of option -read-
                     type. The message digest computed for data read from the
                     attached channel is written into it after the attached
                     channel was closed.  The option is ignored if the channel
                     is write-only.
                     Note that using a variable may yield incorrect results
                     under tcl 7.6, due to embedded \0's.
       The options listed below are always understood by the digest, attached
       versus immediate does not matter. See section IMMEDIATE versus ATTACHED
       for explanations of these two terms.
              -attach channel
                     The presence/absence of this option determines the main
                     operation mode of the transformation.
                     If present the transformation will be stacked onto the
                     channel whose handle was given to the option and run in
                     attached mode. More about this in section IMMEDIATE
                     versus ATTACHED.
                     If the option is absent the transformation is used in
                     immediate mode and the options listed below are
                     recognized. More about this in section IMMEDIATE versus
                     ATTACHED.
              -in channel
                     This options is legal if and only if the transformation
                     is used in immediate mode. It provides the handle of the
                     channel the data to transform has to be read from.
                     If the transformation is in immediate mode and this
                     option is absent the data to transform is expected as the
                     last argument to the transformation.
              -out channel
                     This options is legal if and only if the transformation
                     is used in immediate mode. It provides the handle of the
                     channel the generated transformation result is written
                     to.
                     If the transformation is in immediate mode and this
                     option is absent the generated data is returned as the
                     result of the command itself.
NOTES
       This command uses the same CRC polynomial as the CRC algorithm used by
       PGP (http://www.pgp.com).
IMMEDIATE VERSUS ATTACHED
       The transformation distinguishes between two main ways of using it.
       These are the immediate and attached operation modes.
       For the attached mode the option -attach is used to associate the
       transformation with an existing channel. During the execution of the
       command no transformation is performed, instead the channel is changed
       in such a way, that from then on all data written to or read from it
       passes through the transformation and is modified by it according to
       the definition above.  This attachment can be revoked by executing the
       command unstack for the chosen channel. This is the only way to do this
       at the Tcl level.
       In the second mode, which can be detected by the absence of option
       -attach, the transformation immediately takes data from either its
       commandline or a channel, transforms it, and returns the result either
       as result of the command, or writes it into a channel.  The mode is
       named after the immediate nature of its execution.
       Where the data is taken from, and delivered to, is governed by the
       presence and absence of the options -in and -out.  It should be noted
       that this ability to immediately read from and/or write to a channel is
       an historic artifact which was introduced at the beginning of Trf's
       life when Tcl version 7.6 was current as this and earlier versions have
       trouble to deal with \0 characters embedded into either input or
       output.
SEE ALSO
       adler, crc, crc-zlib, haval, md2, md5, md5_otp, ripemd-128, ripemd-160,
       sha, sha1, sha1_otp, trf-intro
KEYWORDS
       authentication, crc, hash, hashing, mac, message digest, pgp
COPYRIGHT
       Copyright (c) 1996-2003, Andreas Kupries <[email protected]>
Trf transformer commands             2.1.4                              crc(n)

Copy link
Member

@anjannath anjannath Feb 14, 2025

Choose a reason for hiding this comment

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

additionally providing the manpage section number seems to access the correct crc manpage,
this works: man -P cat 1 crc

I added this instead of directly checking the command name because man wasn't discovering the ~/.local/share/man/man1 directory on the QE lab instance.

this seems to be working locally on fedora, so maybe we can investigate and fix it also for the QE lab machine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I've updated it as per your suggestion.

"crc-config-get.1.gz", "crc-ip.1.gz", "crc-version.1.gz",
"crc-config-set.1.gz", "crc-oc-env.1.gz", "crc.1.gz",
"crc-config-unset.1.gz", "crc-podman-env.1.gz",
"crc-config-view.1.gz", "crc-setup.1.gz",
Copy link
Member

Choose a reason for hiding this comment

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

and we could also use Scenario examples to hold this list of manpages, that way its part of the feature file and easy to add new ones, e.g:

Scenario Outline: CRC config checks (settings)
When setting config property "<property>" to value "<value1>" succeeds
Then file "crc.json" exists in CRC home folder
And "JSON" config file "crc.json" in CRC home folder contains key "<property>" with value matching "<value1>"
And setting config property "<property>" to value "<value2>" fails
When unsetting config property "<property>" succeeds
Then "JSON" config file "crc.json" in CRC home folder does not contain key "<property>"
# always return to default values
@darwin
Examples: Config settings on Mac
| property | value1 | value2 |
| cpus | 5 | 3 |
| memory | 10753 | 4096 |
| nameserver | 120.0.0.1 | 999.999.999.999 |
| pull-secret-file | /etc | /nonexistent-file |

Copy link
Contributor Author

@rohanKanojia rohanKanojia Feb 14, 2025

Choose a reason for hiding this comment

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

I was trying this approach, but it has slower execution time as crc setup is being executed for every scenario execution.

Do you know if it's possible to make it execute only once?

@story_manpages
Feature: Check generation and cleanup of manpages

  Background:
    Given executing single crc setup command succeeds

  @linux @darwin
  Scenario Outline: verify man pages are accessible after setup
    Then executing "man -P cat <crc-subcommand>" succeeds
    When executing crc cleanup command succeeds
    Then executing "man -P cat <crc-subcommand>" fails

    @linux @darwin
    Examples: Man pages to check
      | crc-subcommand       |
      | crc-bundle-generate  |
      | crc-config           |
      | crc-start            |
      | crc-bundle           |
      | crc-console          |
      | crc-status           |
      | crc-cleanup          |
      | crc-delete           |
      | crc-stop             |
      | crc-config-get       |
      | crc-ip               |
      | crc-version          |
      | crc-config-set       |
      | crc-oc-env           |
      | crc                  |
      | crc-config-unset     |
      | crc-podman-env       |
      | crc-config-view      |
      | crc-setup            |

Copy link
Member

Choose a reason for hiding this comment

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

We could split it into multiple scenarios, one Scenario where we run just crc setup and then a Scenario Outline with the examples, and then another Scenario for just crc cleanup and lastly a Scenario Outline to check the failure case examples.. but this feels messy, maybe @lilyLuLiu or @adrianriobo knows the right approach to do this

Copy link
Member

Choose a reason for hiding this comment

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

Timing wise with this approach the test time locally for this features is:

19 scenarios (19 passed)
76 steps (76 passed)
4m3.008155834s
ok      github.com/crc-org/crc/v2/test/e2e      244.039s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could split it into multiple scenarios, one Scenario where we run just crc setup and then a Scenario Outline with the examples, and then another Scenario for just crc cleanup and lastly a Scenario Outline to check the failure case examples

Thanks for suggestion, I was able to get this approach working. It executes in 15s

Copy link

openshift-ci bot commented Feb 14, 2025

@rohanKanojia: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-crc 0426c1c link true /test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

Successfully merging this pull request may close these issues.

man pages didn't cleaned after crc cleanup
2 participants