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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in features/aruba #452

Merged
merged 8 commits into from
Jul 23, 2017
Merged

Fix bugs in features/aruba #452

merged 8 commits into from
Jul 23, 2017

Conversation

maxmeyer
Copy link
Member

@maxmeyer maxmeyer commented Jul 12, 2017 •
edited
Loading

Summary

Fix CI errors.

Details

This PR fixes errors on CI to make the build pass again.

Motivation and Context

Make the build pass again.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (cleanup of codebase withouth changing any existing functionality)

Checklist:

  • I've added tests for my code
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@maxmeyer
Copy link
Member Author

maxmeyer commented Jul 12, 2017 •
edited
Loading

@mvz Feel free to push to this PR/branch feature/fix-feature-tests.

mvz
mvz reviewed Jul 13, 2017
View reviewed changes
features/04_aruba_api/core/expand_path.feature Outdated
@@ -69,7 +69,7 @@ Feature: Expand paths with aruba
RSpec.describe 'Expand path', :type => :aruba do
let(:path) { '~/path/to/dir' }

it { expect(expand_path(path)).to match %r</home/[^/]+/path/to/dir> }
it { expect(expand_path(path)).to match %r</home/([^/]+/)+path/to/dir> }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intent here was for [^/]+ to match exacly a user name.

The correct fix is probably to remove the 'before 1.0.0' part of the feature and fix the 1.0.0 part to not set things up in the before section.

Copy link
Member Author

@maxmeyer maxmeyer Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, I think, it was a "hack" I introduced. But you're right, the correct way is to remove that test. I missed the comment last night. Thanks.

@mvz
Copy link
Contributor

mvz commented Jul 16, 2017

Some of the remaining failures are due to Aruba not waiting for the running command to stop. @maxmeyer can you advise on what the desired behavior should be? I'm talking about cases where right now the cucumber scenarios have this:

  Scenario: Running aruba interactively

    This starts a pry console with "aruba"'s api loaded.

    When I run `bin/console` interactively
    And I type "exit"
    Then the exit status should be 0

The last step does not wait for any command to finish, unlike the old pre-1.0 behavior.

@maxmeyer
Copy link
Member Author

@mvz

  • First of all we've got an issue for this behaviour: Regression: exit status not waiting for run interactively process to complete #432
  • I thought about making stopping a command more explicit in aruba 1.0.0 or at least in 2.0.0, with a cucumber step, but I think that need's to be discussed upfront
  • Personally I would expect aruba to wait a couple of seconds before stopping the command and checking the status
  • This needs to work both with interactive and non-interactive commands

The last step does not wait for any command to finish, unlike the old pre-1.0 behavior.

Unfortunately I think I broke this step "a long time ago" without having issues on CI for a while. At least the code in several parts which are involved is the same for quite a long time.

What do you think about this?

@maxmeyer
Copy link
Member Author

@mvz I forced pushed a rebased version of this branch. Just for you to know. I removed the if-statements along with the changed tests.

@maxmeyer
Copy link
Member Author

@mvz I would like to merge this branch and then setup another one to capture the discussion around the problems with failing tests due to aruba not waiting for the command to finish.

@mvz
Copy link
Contributor

mvz commented Jul 23, 2017

@maxmeyer sounds like a good plan.

mvz
mvz previously approved these changes Jul 23, 2017
View reviewed changes
Copy link
Contributor

@mvz mvz left a comment

Choose a reason for hiding this comment

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

:shipit:

@maxmeyer maxmeyer force-pushed the feature/fix-feature-tests branch 2 times, most recently from 3d3e8d0 to b2cc4dd Compare July 23, 2017 09:29
@maxmeyer maxmeyer merged commit 51a034a into master Jul 23, 2017
@maxmeyer maxmeyer deleted the feature/fix-feature-tests branch July 24, 2017 08:15
@maxmeyer maxmeyer added this to the 1.0.0-alpha.3 milestone Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvz mvz mvz left review comments

Assignees
No one assigned
Labels
status: work in progress
Projects
None yet
Milestone
1.0.0-alpha.5
Development

Successfully merging this pull request may close these issues.
2 participants
@maxmeyer @mvz