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: Randomization script refactoring #5914

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@mrsolo
Copy link
Contributor

mrsolo commented Apr 22, 2014

  1. Refactored randomization logic to the top
  2. Add logics to support window platforms

@mrsolo mrsolo added the review label Apr 22, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Apr 23, 2014

I am not an ruby expert maybe @karmi can take a look here?

@karmi

View changes

dev-tools/build_randomization.rb Outdated
require 'yaml'


This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

It would be better to remove the extra empty lines (not only because of Git merging heuristics).

@karmi

View changes

dev-tools/build_randomization.rb Outdated
# an 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
# either express or implied. See the License for the specific
# language governing permissions and limitations under the License

#
# generate property file for the jdk randomization test
#
# generate property file for the JDK randomization test

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

It would be nice to add example usage here in the script preamble.

@karmi

View changes

dev-tools/build_randomization.rb Outdated

RANDOM_CHOICES = {
'tests.jvm.argline' => [
{:choices => ['-server'], :method => :get_random_one},

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

What about using Ruby 1.9 Hash syntax to improve readability? Ie:

choices: ['-server'], method: :get_random_one

or, preferably, have the value as String:

choices: ['-server'], method: 'get_random_one'

This comment has been minimized.

Copy link
@mrsolo

mrsolo Apr 29, 2014

Author Contributor

Unfortunately the script has to be 1.8.7 compatible for now (Centos testbed)

@karmi

View changes

dev-tools/build_randomization.rb Outdated
'tests.security.manager' => {:choices => [true, false], :method => :get_90_percent},
}

L = Logger.new ''

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

So, if we use "Log4R" (and not the default logger), let's use the features and give this log a name? "test_randomizer" or something like that?

@karmi

View changes

dev-tools/build_randomization.rb Outdated

C = {:local => false, :test => false}

opts = GetoptLong.new(

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

I'm not familiar with GetOptLong, I guess most Ruby people would use and understand optparse. Could it be used instead? (Added bonus, it would be more similar to Python's optparse I guess...)

@karmi

View changes

dev-tools/build_randomization.rb Outdated
class Randomizer

def true_or_false(*params)
[true, false][rand(2)]

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

In Ruby > 1.9, it would be more readable to use the Array#sample method: [true, false].sample.

@karmi

View changes

dev-tools/build_randomization.rb Outdated

class Randomizer

def true_or_false(*params)

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

Since the params are not used, I'd just remove them, to avoid confusion.

@karmi

View changes

dev-tools/build_randomization.rb Outdated
def get_random_with_distribution(data_array, distribution)
carry = 0
# new api not compatible with older version of ruby
#distribution_map = distribution.map.with_index {|x, i| pre_carry = carry ; carry += x; {i => x + pre_carry} }

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

Commented out code should be removed without remorse :)

@jdk_list = Dir.entries(directory).select do |x|
x.chars.first == 'J'
@jdk_list = Dir.entries(directory).select do |x|
x.chars.first == 'J'

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

Is this enough to detect a JDK? What about a more rigid regex?

This comment has been minimized.

Copy link
@mrsolo

mrsolo Apr 29, 2014

Author Contributor

Yes since our Jenkins controls the directory naming :-)

@karmi

View changes

dev-tools/build_randomization.rb Outdated

get_random_one(selection_array)
def JDKSelector.generate_jdk_hash(jdk_choice)
file_seperator = if Gem.win_platform?

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

Should be file_separator probably, to avoid future typos?

@karmi

View changes

dev-tools/build_randomization.rb Outdated
configuration.each do |k, v|
randomizer = Randomizer.new
if(v.kind_of?(Hash))
v[:selections] = randomizer.send(v[:method], v[:choices]) if v.has_key?(:method)

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

Let's use __send__ to be on the safe side, and signal the intent ("gonna call a method dynamically") better.

@karmi

View changes

dev-tools/build_randomization.rb Outdated
#
# Main
#
unless(C[:test])

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

What about using environment variable TEST=true instead, so the main routine is simpler? Do you have to manually change the C hash to run tests? -- I see this is handled by optparse.

@karmi

View changes

dev-tools/build_randomization.rb Outdated
data_array[choice]
end

def get_random_one(data_array)

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

Instead of passing the data_array as a method argument, it would probably be better to initialize the Randomizer class with it, and work with it as instance variable?

@karmi

View changes

dev-tools/build_randomization.rb Outdated


# generated environment hash table based on JDK and randomized selections
def get_env_matrix(jdk_selection, selections)

This comment has been minimized.

Copy link
@karmi

karmi Apr 24, 2014

Member

These methods kinda hang in the main scope here, what about putting them to the Randomizer class, or abstract the classes a bit more and put it into some "main" class, like RandomizedRunner or something like that?

Then the main executable code around line 300, can just call RandomizedRunner.new(some arguments).run!?

@karmi

This comment has been minimized.

Copy link
Member

karmi commented Apr 24, 2014

I think the patch improves on the current Ruby script a lot, I like the extraction of logic into classes and particularly the bundled unit tests, because any bugs can be covered by them once discovered in production. I think it's safe to merge.

I did mostly style-related comments, I think in the future the code can be abstracted even a bit more, so it has a single entry point for executable code (RandomizedRunner.new(some, args, ...).run!).

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jun 12, 2014

@mrsolo can we bring this forward?

@s1monw s1monw added review and removed review labels Jun 12, 2014

@mrsolo

This comment has been minimized.

Copy link
Contributor Author

mrsolo commented Jun 12, 2014

File update incorporating some of the comments. Unfortunately, this script has to be 1.8 compatible. Adding some additional logic to take account of window platform variations (future refactoring target)

@karmi

View changes

dev-tools/build_randomization.rb Outdated
# The key of randomization hash maps to key of java property. The value of the hash describes the possible value of the randomization
#
# For example RANDOM_CHOICES = { 'es.node.mode' => {:choices => ['local', 'network'], :method => :get_random_one} } means
# es.node.mode will be set to either 'local' or 'network', each with 50% of probability

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

I still miss the "usage" example for the script here...

@karmi

View changes

dev-tools/build_randomization.rb Outdated
end.parse!

class Randomizer

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Can we remove the empty line?

@karmi

View changes

dev-tools/build_randomization.rb Outdated
end

def get_random_with_distribution(mdata_array, distribution)
L.debug("randomized distribution data %s"%YAML.dump(mdata_array))

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Please add space to increase legibility, randomized distribution data %s" % YAML.dump

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Also, the ( parentheses are optional here, would be more readable like this?

L.debug "randomized distribution data %s" % YAML.dump(mdata_array)
@karmi

View changes

dev-tools/build_randomization.rb Outdated
end

def method_missing(meth, *args, &block)

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Empty line should be removed.

@karmi

View changes

dev-tools/build_randomization.rb Outdated
# Fix argument JDK selector
#
class FixedJDKSelector < JDKSelector

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Again would be better to remove the empty line.

@karmi

View changes

dev-tools/build_randomization.rb Outdated

# # pick first element out of array of hashes, generate write java property file
def generate_property_file(data)

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Empty line... :)

@karmi

View changes

dev-tools/build_randomization.rb Outdated
#
# Execute randomization logics
#
class RandomizeRunner

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Isn't this RandomizedRunner? (Note the "d"...)

@karmi

View changes

dev-tools/build_randomization.rb Outdated
end

def generate_selections

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Empty line...

@karmi

View changes

dev-tools/build_randomization.rb Outdated
L.debug "Configuration %s"%YAML.dump(configuration)

generated = {}
configuration.each do |k, v|

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

I'd probably indent this very long block differently, to increase readability (but that is a subjective quality :)

configuration.

  each do |k, v|
    if(v.kind_of?(Hash))
      if(v.has_key?(:method))
        randomizer = Randomizer.new(v[:choices])
        v[:selections] = randomizer.__send__(v[:method])
      end
    else
      v.each do |x|
        if(x.has_key?(:method))
          randomizer = Randomizer.new(x[:choices])
          x[:selections] = randomizer.__send__(x[:method])
        end
      end
    end
  end.

  each do |k, v|
    if(v.kind_of?(Array))
      selections = v.inject([]) do |sum, current_hash|
        sum.push(current_hash[:selections])
      end
    else
      selections = [v[:selections]] unless v[:selections].nil?
    end

    generated[k] = selections unless (selections.nil? || selections.size == 0)
  end

(Ruby 1.8.7 compatible.)

This comment has been minimized.

Copy link
@mrsolo

mrsolo Jul 10, 2014

Author Contributor

Emacs gets very confused about this type of formatting... It doesn't handle statement that ends in '.' very well.

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Weird, but no worries!

@karmi

View changes

dev-tools/build_randomization.rb Outdated
def test_initialize
['/home/dummy','/tmp'].each do |x|
test_object = PropertyWriter.new(x)
assert(test_object.working_directory.kind_of?(String))

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

This should be assert_kind_of.

@karmi

View changes

dev-tools/build_randomization.rb Outdated
['/home/dummy','/tmp'].each do |x|
test_object = PropertyWriter.new(x)
assert(test_object.working_directory.kind_of?(String))
assert(test_object.working_directory == x)

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

This should be assert_equal x, test_object.working_directory. As in previous line, the failed assertion has much more informative message then.

@karmi

View changes

dev-tools/build_randomization.rb Outdated

def test_initialize
test_object = RandomizeRunner.new(RANDOM_CHOICES, '/tmp/dummy/jdk', po = PropertyWriter.new('/tmp'))
assert(test_object.random_choices == RANDOM_CHOICES)

This comment has been minimized.

Copy link
@karmi

karmi Jul 10, 2014

Member

Of course, same note about assert_equal here and everywhere appropriate... assert ... == ... is an antipattern.

@mrsolo

This comment has been minimized.

Copy link
Contributor Author

mrsolo commented Jul 10, 2014

File updated, incorporate comments

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Jul 11, 2014

@karmi please could you review this again and give a LGTM if it's ok to get in for 1.3?


def get_random_with_distribution(mdata_array, distribution)
L.debug "randomized distribution data %s" % YAML.dump(mdata_array)
L.debug "randomized distribution distribution %s" % YAML.dump(distribution)

This comment has been minimized.

Copy link
@karmi

karmi Jul 11, 2014

Member

Great, thanks for the spaces :)

@karmi

View changes

dev-tools/build_randomization.rb Outdated
end.sort
file_name = (ENV['BUILD_ID'] + ENV['BUILD_NUMBER']) || 'prop' rescue 'prop'
file_name = file_name.split(File::SEPARATOR).first + '.txt'
L.debug "Property file name is %s"%file_name

This comment has been minimized.

Copy link
@karmi

karmi Jul 11, 2014

Member

@mrsolo Could you please grep the file for "% and add the spaces to this interpolation notation?

@karmi

View changes

dev-tools/build_randomization.rb Outdated
# Check to see if this is running locally
unless(C[:local])
L.debug("Normal Mode")
working_directory = ENV['WORKSPACE'] || if(Gem.win_platform?)

This comment has been minimized.

Copy link
@karmi

karmi Jul 11, 2014

Member

This could be also written as working_directory = ENV.fetch 'WORKSPACE', (Gem.win_platform? ? Dir.pwd : '/var/tmp') -- the ENV.fetch "something", "default" is a nice pattern.

This comment has been minimized.

Copy link
@mrsolo

mrsolo Jul 11, 2014

Author Contributor

Nice..make the code cleaner for sure

content = data.to_a.map do |x|
x.join('=')
end.sort
file_name = (ENV['BUILD_ID'] + ENV['BUILD_NUMBER']) || 'prop' rescue 'prop'

This comment has been minimized.

Copy link
@karmi

karmi Jul 11, 2014

Member

(ENV['BUILD_ID'] + ENV['BUILD_NUMBER']) rescue 'prop' seems enough, since the || won't ever be evaluated?, it'll always explode on can't convert nil into String when the ENV vars are not set?

This comment has been minimized.

Copy link
@mrsolo

mrsolo Jul 11, 2014

Author Contributor

This is Jenkins specific issue and how we use it. As I recall, on parent job, there was a scenario BUILD_* exists but not set.

Things may have changed. This is worth a revisit when next round of refactoring occur.

@karmi

This comment has been minimized.

Copy link
Member

karmi commented Jul 11, 2014

@mrsolo @clintongormley I've left couple of notes in the diff, but those are again mostly cosmetic, it's definitely OK to include it in 1.3 -- it's used on Jenkins anyway, as it is.

@mrsolo

This comment has been minimized.

Copy link
Contributor Author

mrsolo commented Jul 11, 2014

Updated with latest comments; will proceed with merge.

@s1monw s1monw added v1.4.0 and removed v1.3.0 labels Jul 14, 2014

[BUILD] Randomization script refactoring
1) Refactored randomization logic to the top
2) Add logics to support window platforms
@mrsolo

This comment has been minimized.

Copy link
Contributor Author

mrsolo commented Jul 16, 2014

Merged

Master
bdf77fe

1.x
c16bc36

@mrsolo mrsolo closed this Jul 16, 2014

@jpountz jpountz removed the review label Jul 17, 2014

@clintongormley clintongormley changed the title [BUILD] Randomization script refactoring Test: Randomization script refactoring Sep 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.