Update tests for 'newer' Windows builds #2348

MSP-Greg · 2018-07-07T18:24:01Z

Description:

Two test files have 'Windows' skips that are not required for newer versions of Ruby when Windows is run with full admin access (as is the case with Appveyor).

Updated the tests, and tested locally as a non-admin, tests then skip.

Note: one test (test_extract_symlink_parent) that was updated had a single space indent instead of two. Corrected the indent for the whole test.

Tasks:

Describe the problem / feature
Write tests
Write code to solve the problem
Get code review from coworkers / friends

I will abide by the code of conduct.

MSP-Greg · 2018-07-07T19:00:08Z

Change in test results:

Ruby 2.2.6
pre:  2022 runs, 5958 assertions, 0 failures, 0 errors, 39 skips
post: 2022 runs, 5995 assertions, 0 failures, 0 errors, 31 skips

Trunk
pre:  2022 runs, 5963 assertions, 0 failures, 0 errors, 39 skips
post: 2022 runs, 6007 assertions, 0 failures, 0 errors, 28 skips

hsbt

Thanks. Please consider my comments.

hsbt · 2018-07-07T20:47:38Z

test/rubygems/test_gem_gemcutter_utilities.rb

@@ -179,7 +179,7 @@ def test_sign_in_with_other_credentials_doesnt_overwrite_other_keys
  end

  def test_sign_in_with_bad_credentials
-    skip 'Always uses $stdin on windows' if Gem.win_platform?
+    skip 'Always uses $stdin on windows' if Gem.win_platform? && RUBY_VERSION <= '1.9.2'


Please remove this line completely, Because the master branch only supports Ruby2.2+

hsbt · 2018-07-07T20:48:00Z

test/rubygems/test_gem_gemcutter_utilities.rb

@@ -190,7 +190,7 @@ def test_sign_in_with_bad_credentials
  end

  def util_sign_in response, host = nil, args = []
-    skip 'Always uses $stdin on windows' if Gem.win_platform?
+    skip 'Always uses $stdin on windows' if Gem.win_platform? && RUBY_VERSION <= '1.9.2'


Same as L182.

hsbt · 2018-07-07T20:50:03Z

test/rubygems/test_gem_package.rb

@@ -472,28 +480,32 @@ def test_extract_tar_gz_symlink_relative_path
  end

  def test_extract_symlink_parent


Can you revert stylistic changes? We shouldn't mix it and feature changes.

Respectfully, no. I will remove the above two code lines. To not waste another set of tests, I'll do so when this is resolved.

Given what I've seen in this repo:

If I submitted a PR with a new test and the 1st indent within the method def was one space, would I be asked to change it?

I've aligned the formatting of one test in a file of 16, among over two thousand test methods in total.

I've seen maintainers/members discuss things they admit to never having used, want benchmarks that had so many parameters affecting them as to make them pointless, and discuss engine/platform configurations they have never used anything comparable to. I've also seen them not respond to very specific questions.

I'm just trying to make RubyGems better for myself and also users in general (including new users). I've been writing code since I was a teenager using an acoustic-coupler and a teletype. I started in CompSci before changing to EE/Math. IOW, I'm old enough to say:

Y'all need to quit badgering contributors and let them improve RubyGems

Sorry for the rant...

Unfortunately, I will never merge this until you fixed them. It's an elementary request in pull request of GitHub.

See No.4 in http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/.

See No.4

And No.4's first paragraph is: "You may feel the urge to change the formatting of the existing code to fit 'your' style. Please abstain."

I did not do that. I formatted a test method I was already changing to match all the other test methods. My editor's ruby configuration replaces a tab with two spaces. and the non-standard spacing made it a PITA to edit. So I fixed it.

Unfortunately, I will never

Then I guess we're at an impasse...

@MSP-Greg @hsbt I opened #2350 to unblock this!

@MSP-Greg If you rebase now everyone should be happy I think! 💪

Thanks. Conflict with the rebase, just a bit more work. Thanks, Greg

colby-swandale · 2018-07-08T01:40:18Z

test/rubygems/test_gem_package.rb

+    begin
+      File.symlink('code.rb', 'lib/code_sym.rb')
+    rescue Errno::EACCES
+      skip "symlink - must be admin with no UAC on Windows"


There should be a if Gem.win_platform?, otherwise this skip could show up on other platforms where this message does not make much sense to the user.

If it hits the rescue and it's not windows, what should happen? Nothing, raise the error, or something else?

I only added it because many windows desktops (including mine) will raise the error unless testing is run from an 'admin prompt/console'. Servers are normally configured differently. On Appvyor, the tests do not skip.

I think that it should raise so that the exception propagates back up to the user.

@colby

Rebased, and also added similar logic to line 514. And yeah, I screwed up offsetting line 513...

colby-swandale · 2018-07-08T01:40:57Z

test/rubygems/test_gem_package.rb

+    begin
+      package.extract_tar_gz tgz_io, @destination
+    rescue Errno::EACCES
+      skip "symlink - must be admin with no UAC on Windows"


Same a previous comment

bundlerbot · 2018-07-08T22:33:23Z

☔ The latest upstream changes (presumably #2350) made this pull request unmergeable. Please resolve the merge conflicts.

hsbt · 2018-07-09T13:47:26Z

@bundlerbot r+

bundlerbot · 2018-07-09T13:47:27Z

📌 Commit 2e3dbb0 has been approved by hsbt

bundlerbot · 2018-07-09T13:47:38Z

⌛ Testing commit 2e3dbb0 with merge c81a917...

Update tests for 'newer' Windows builds # Description: Two test files have 'Windows' skips that are not required for newer versions of Ruby when Windows is run with full admin access (as is the case with Appveyor). Updated the tests, and tested locally as a non-admin, tests then skip. Note: one test (test_extract_symlink_parent) that was updated had a single space indent instead of two. Corrected the indent for the whole test. # Tasks: - [X] Describe the problem / feature - [X] Write tests - [X] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

bundlerbot · 2018-07-09T14:13:59Z

☀️ Test successful - status-travis
Approved by: hsbt
Pushing c81a917 to master...

…wandale Normalize comment indentations # Description: This is a minor follow up to #2350, that fixes another style inconsistency that showed up in #2348. ______________ # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

bronzdoc added the status: triage label Jul 7, 2018

hsbt requested changes Jul 7, 2018

View reviewed changes

colby-swandale reviewed Jul 8, 2018

View reviewed changes

MSP-Greg force-pushed the fix_windows_skips branch from 687ff1e to 6353c40 Compare July 8, 2018 13:44

Update tests for 'newer' Windows builds

2e3dbb0

MSP-Greg force-pushed the fix_windows_skips branch from 6353c40 to 2e3dbb0 Compare July 8, 2018 23:29

hsbt approved these changes Jul 9, 2018

View reviewed changes

bundlerbot merged commit 2e3dbb0 into rubygems:master Jul 9, 2018

bronzdoc removed the status: triage label Jul 9, 2018

MSP-Greg deleted the fix_windows_skips branch July 9, 2018 14:42

deivid-rodriguez mentioned this pull request Jul 9, 2018

Normalize comment indentations #2353

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tests for 'newer' Windows builds #2348

Update tests for 'newer' Windows builds #2348

MSP-Greg commented Jul 7, 2018 •

edited

MSP-Greg commented Jul 7, 2018 •

edited

hsbt left a comment

hsbt Jul 7, 2018

hsbt Jul 7, 2018

hsbt Jul 7, 2018

MSP-Greg Jul 7, 2018

hsbt Jul 8, 2018 •

edited

MSP-Greg Jul 8, 2018 •

edited

deivid-rodriguez Jul 8, 2018

deivid-rodriguez Jul 8, 2018

MSP-Greg Jul 8, 2018

colby-swandale Jul 8, 2018

MSP-Greg Jul 8, 2018

colby-swandale Jul 8, 2018

MSP-Greg Jul 8, 2018

colby-swandale Jul 8, 2018

bundlerbot commented Jul 8, 2018

hsbt commented Jul 9, 2018

bundlerbot commented Jul 9, 2018

bundlerbot commented Jul 9, 2018

bundlerbot commented Jul 9, 2018

		@@ -472,28 +480,32 @@ def test_extract_tar_gz_symlink_relative_path
		end

		def test_extract_symlink_parent

Update tests for 'newer' Windows builds #2348

Update tests for 'newer' Windows builds #2348

Conversation

MSP-Greg commented Jul 7, 2018 • edited

Description:

Tasks:

MSP-Greg commented Jul 7, 2018 • edited

hsbt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsbt Jul 8, 2018 • edited

Choose a reason for hiding this comment

MSP-Greg Jul 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bundlerbot commented Jul 8, 2018

hsbt commented Jul 9, 2018

bundlerbot commented Jul 9, 2018

bundlerbot commented Jul 9, 2018

bundlerbot commented Jul 9, 2018

MSP-Greg commented Jul 7, 2018 •

edited

MSP-Greg commented Jul 7, 2018 •

edited

hsbt Jul 8, 2018 •

edited

MSP-Greg Jul 8, 2018 •

edited