From 7ac1b7c1b127794556c8fe2967da3bd2ed0a32e2 Mon Sep 17 00:00:00 2001 From: Luke Hill <20105237+luke-hill@users.noreply.github.com> Date: Wed, 16 Oct 2019 10:31:36 +0100 Subject: [PATCH] Refactor/rubocop autofixes on selenium rake (#7671) * Autofixes for rake_tasks folder * Massively cull the unused keys of the Browsers hash * Slight drying up of checks class * Dry up checks to only retain the checker methods we use * Avoid frozenstringliteral issues by using squiggly heredocs *Also tidies up the very systemic creation of starting points of c++/java files * Dry up class usages by using Decorator approach and pass class instance directly in (By default for detonatinghandler) * Fix up a couple of areas that would complain due to frozen_string failures * Reduce complexity of crazy-fun methodology, and tidy up some of the formatter tabbing * Slightly tidy up checks for present? to be a bit more comprehensible * Begin to remove un-required code * c_checks no longer required * BROWSERS props not consumed --- Rakefile | 5 +- rake_tasks/selenium_rake/base_generator.rb | 4 + rake_tasks/selenium_rake/browsers.rb | 73 +++------------ rake_tasks/selenium_rake/c_tasks.rb | 91 ------------------- rake_tasks/selenium_rake/checks.rb | 78 +++++++--------- rake_tasks/selenium_rake/cpp_formatter.rb | 13 ++- rake_tasks/selenium_rake/crazy_fun.rb | 56 ++++++++---- .../selenium_rake/detonating_handler.rb | 2 + rake_tasks/selenium_rake/ie_code_generator.rb | 2 + rake_tasks/selenium_rake/java_formatter.rb | 13 ++- .../type_definitions_generator.rb | 8 +- 11 files changed, 110 insertions(+), 235 deletions(-) delete mode 100644 rake_tasks/selenium_rake/c_tasks.rb diff --git a/Rakefile b/Rakefile index b62fc62d91b15..26bc557827cb6 100644 --- a/Rakefile +++ b/Rakefile @@ -27,7 +27,6 @@ require 'rake_tasks/crazy_fun/mappings/rename' require 'rake_tasks/crazy_fun/mappings/ruby' # Location of all new methods -require 'rake_tasks/selenium_rake/c_tasks' require 'rake_tasks/selenium_rake/checks' require 'rake_tasks/selenium_rake/ie_code_generator' require 'rake_tasks/selenium_rake/java_formatter' @@ -213,8 +212,8 @@ task :test_java_webdriver => [ ] task :test_java_webdriver => [:test_ie] if SeleniumRake::Checks.windows? -task :test_java_webdriver => [:test_chrome] if SeleniumRake::Checks.present?("chromedriver") -task :test_java_webdriver => [:test_edge] if SeleniumRake::Checks.present?("msedgedriver") +task :test_java_webdriver => [:test_chrome] if SeleniumRake::Checks.chrome? +task :test_java_webdriver => [:test_edge] if SeleniumRake::Checks.edge? task :test_java_webdriver => [:test_opera] if SeleniumRake::Checks.opera? task :test_java => [ diff --git a/rake_tasks/selenium_rake/base_generator.rb b/rake_tasks/selenium_rake/base_generator.rb index 419a9f0089c8a..9f1c0025c5314 100644 --- a/rake_tasks/selenium_rake/base_generator.rb +++ b/rake_tasks/selenium_rake/base_generator.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module SeleniumRake class BaseGenerator def create_deps_(out, args) @@ -14,6 +16,8 @@ def create_deps_(out, args) t.out = out end + private + def add_deps_(task_name, srcs) return if srcs.nil? diff --git a/rake_tasks/selenium_rake/browsers.rb b/rake_tasks/selenium_rake/browsers.rb index 0509ae6171d79..873b7999db325 100644 --- a/rake_tasks/selenium_rake/browsers.rb +++ b/rake_tasks/selenium_rake/browsers.rb @@ -1,73 +1,30 @@ +# frozen_string_literal: true + require 'rake_tasks/selenium_rake/checks' module SeleniumRake class Browsers BROWSERS = { 'ff' => { - python: { - driver: 'Marionette' - }, - java: { - class: 'org.openqa.selenium.firefox.SynthesizedFirefoxDriver', - deps: ['//java/client/test/org/openqa/selenium/testing/drivers'] - }, - browser_name: 'firefox' + python: { driver: 'Marionette' }, }, 'marionette' => { - python: { - driver: 'Marionette' - }, - java: { - class: 'org.openqa.selenium.firefox.SynthesizedFirefoxDriver', - deps: ['//java/client/test/org/openqa/selenium/testing/drivers'] - }, - browser_name: 'firefox' + python: { driver: 'Marionette' }, }, 'ie' => { - python: { - driver: 'Ie' - }, - java: { - class: 'org.openqa.selenium.ie.InternetExplorerDriver', - deps: ['//java/client/src/org/openqa/selenium/ie:ie', '//cpp/iedriverserver:win32'] - }, - browser_name: 'internet explorer', - available: SeleniumRake::Checks.windows? + python: { driver: 'Ie' }, }, 'edge' => { - python: { - driver: 'Edge' - }, - browser_name: 'MicrosoftEdge', - available: SeleniumRake::Checks.windows? + python: { driver: 'Edge' }, }, 'chrome' => { - python: { - driver: 'Chrome' - }, - java: { - class: 'org.openqa.selenium.chrome.ChromeDriver', - deps: ['//java/client/src/org/openqa/selenium/chrome:chrome'] - }, - browser_name: 'chrome', - available: SeleniumRake::Checks.chrome? + python: { driver: 'Chrome' }, }, 'chromiumedge' => { - python: { - driver: 'ChromiumEdge' - }, - java: { - class: 'org.openqa.selenium.edge.EdgeDriver', - deps: ['//java/client/src/org/openqa/selenium/edge:edge'] - }, - browser_name: 'MicrosoftEdge', - available: SeleniumRake::Checks.edge? + python: { driver: 'ChromiumEdge' }, }, 'blackberry' => { - python: { - driver: 'BlackBerry' - }, - browser_name: 'blackberry' + python: { driver: 'BlackBerry' }, }, 'remote_firefox' => { python: { @@ -80,16 +37,8 @@ class Browsers } }, 'safari' => { - python: { - driver: 'Safari' - }, - java: { - class: 'org.openqa.selenium.safari.SafariDriver', - deps: ['//java/client/src/org/openqa/selenium/safari:safari'] - }, - browser_name: 'safari', - available: SeleniumRake::Checks.mac? + python: { driver: 'Safari' }, } - } + }.freeze end end diff --git a/rake_tasks/selenium_rake/c_tasks.rb b/rake_tasks/selenium_rake/c_tasks.rb deleted file mode 100644 index 844e7eb3db97e..0000000000000 --- a/rake_tasks/selenium_rake/c_tasks.rb +++ /dev/null @@ -1,91 +0,0 @@ -module SeleniumRake - class CTasks - attr_accessor :bitness - - def dll(args) - deps = build_deps_(args[:deps]) - result_array = Array(args[:out]) - - result_array.each do |result| - out = "build/#{result}" - - compile_file(out, deps, args) - - # TODO(simon): Yuck. Not Good Enough - task args[:name].to_s => out - task args[:out] => out - Rake::Task[args[:name]].out = out.to_s - end - end - - private - - def compile_file(out, deps, args) - full_build_deps = "#{build_deps_(args[:src])}#{deps}" - - file out => full_build_deps do - if out.end_with?('.dll') - msbuild_legacy(args[:solution], out, args[:prebuilt]) - elsif out.end_with?('.so') - is_32_bit = args[:arch] != 'amd64' - self.bitness = is_32_bit - gcc(args[:src], out, args[:args], args[:link_args], args[:prebuilt]) - else - puts "Cannot compile #{args[:out]}" - exit -1 - end - end - end - - def msbuild_legacy(solution, out, prebuilt) - return copy_prebuilt(prebuilt, out) unless msbuild_installed? - - unless File.exist?(out) - sh "MSBuild.exe #{solution} /verbosity:q /target:Rebuild /property:Configuration=Release /property:Platform=x64", verbose: false - sh "MSBuild.exe #{solution} /verbosity:q /target:Rebuild /property:Configuration=Release /property:Platform=Win32", verbose: false - copy_to_prebuilt(out, prebuilt) - end - end - - def gcc(srcs, out, args, link_args, prebuilt) - return copy_prebuilt(prebuilt, out) unless gcc? - - obj_dir = "#{out}_temp/obj#{bitness}" - - mkdir_p obj_dir - - is_cpp_code = false - srcs.each do |src| - ok = gccbuild_c(src, obj_dir, args) - return copy_prebuilt(prebuilt, out) unless ok - is_cpp_code = true if src.end_with?('.cpp') - end - - flags = "-Wall -shared -fPIC -Os -fshort-wchar -m#{bitness}" - flags += " #{link_args} " if link_args - - # if we've made it this far, try to link. If link fails, copy from prebuilt. - linker = is_cpp_code ? 'g++' : 'gcc' - linker_cmd = "#{linker} -o #{out} #{obj_dir}/*.o #{flags}" - sh linker_cmd do |link_ok| - return copy_prebuilt(prebuilt, out) unless link_ok - end - - copy_to_prebuilt(out, prebuilt) - - rm_rf "#{out}_temp" - end - - def gccbuild_c(src_file, obj_dir, args) - compiler = src_file.end_with?('.c') ? 'gcc' : 'g++' - objname = src_file.split('/')[-1].sub(/\.c[p{2}]*?$/, '.o') - cmd = "#{compiler} #{src_file} -Wall -c -fshort-wchar -fPIC -o #{obj_dir}/#{objname} " - cmd += " -m#{bitness}" - cmd += args if args - sh cmd do |ok| - return puts 'Unable to build. Aborting compilation' unless ok - end - true - end - end -end diff --git a/rake_tasks/selenium_rake/checks.rb b/rake_tasks/selenium_rake/checks.rb index fab3269a2121b..914c0af29a8c1 100644 --- a/rake_tasks/selenium_rake/checks.rb +++ b/rake_tasks/selenium_rake/checks.rb @@ -1,8 +1,12 @@ +# frozen_string_literal: true + require 'rbconfig' module SeleniumRake class Checks class << self + PRESENT_CACHE = {} + def windows? (RbConfig::CONFIG['host_os'] =~ /mswin|msys|mingw32/) != nil end @@ -11,28 +15,12 @@ def mac? (RbConfig::CONFIG['host_os'] =~ /darwin|mac os/) != nil end - def linux? - (RbConfig::CONFIG['host_os'] =~ /linux/) != nil - end - - def cygwin? - RUBY_PLATFORM.downcase.include?('cygwin') - end - def dir_separator File::ALT_SEPARATOR || File::SEPARATOR end - def env_separator - File::PATH_SEPARATOR - end - - def jruby? - RUBY_PLATFORM =~ /java/ - end - def path_for(path) - windows? ? path.gsub("/", File::ALT_SEPARATOR || File::SEPARATOR) : path + windows? ? path.gsub('/', dir_separator) : path end def classpath_separator? @@ -43,26 +31,6 @@ def classpath_separator? end end - PRESENT_CACHE = {} - - # Checking for particular applications - # This "I believe" can be made private - Luke - Sep 2019 - def present?(arg) - return PRESENT_CACHE[arg] if PRESENT_CACHE.key?(arg) - - prefixes = ENV['PATH'].split(File::PATH_SEPARATOR) - - bool = prefixes.any? do |prefix| - File.exist?(prefix + File::SEPARATOR + arg) - end - - bool = File.exist?("/Applications/#{arg}.app") if !bool && mac? - - PRESENT_CACHE[arg] = bool - - bool - end - def chrome? present?('chromedriver') || present?('chromedriver.exe') end @@ -71,15 +39,6 @@ def edge? present?('msedgedriver') || present?('msedgedriver.exe') end - # Think of the confusion if we called this "g++" - def gcc? - linux? && present?('g++') - end - - def msbuild_installed? - windows? && present?('msbuild.exe') - end - def opera? present?('opera') || present?('Opera') end @@ -87,6 +46,33 @@ def opera? def python? present?('python') || present?('python.exe') end + + private + + def cygwin? + RUBY_PLATFORM.downcase.include?('cygwin') + end + + def present?(arg) + return PRESENT_CACHE[arg] if PRESENT_CACHE.key?(arg) + return PRESENT_CACHE[arg] = true if exist_on_non_mac?(arg) + return PRESENT_CACHE[arg] = exist_on_mac?(arg) if mac? + PRESENT_CACHE[arg] = false + end + + def exist_on_non_mac?(arg) + prefixes.any? do |prefix| + File.exist?("#{prefix}#{File::SEPARATOR}#{arg}") + end + end + + def exist_on_mac?(arg) + File.exist?("/Applications/#{arg}.app") + end + + def prefixes + ENV['PATH'].split(File::PATH_SEPARATOR) + end end end end diff --git a/rake_tasks/selenium_rake/cpp_formatter.rb b/rake_tasks/selenium_rake/cpp_formatter.rb index d9d321b0199c5..5a93daa5c6d01 100644 --- a/rake_tasks/selenium_rake/cpp_formatter.rb +++ b/rake_tasks/selenium_rake/cpp_formatter.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This file auto-generates code for the IE driver. To make different language # bindings easier to maintain, it generates code for mapping numeric return # type identifiers (returned from wdGetScriptResultType) to a meaningful @@ -5,11 +7,12 @@ module SeleniumRake class CppFormatter def generate_file_header - out_str = "/* AUTO GENERATED - Do not edit by hand. */\n" - out_str += "/* See rake-tasks/selenium_rake/cpp_formatter.rb instead. */\n" - out_str += "#ifndef __IE_RETURN_TYPES_H_\n" - out_str += "#define __IE_RETURN_TYPES_H_\n" - out_str + <<~HEREDOC + /* AUTO GENERATED - Do not edit by hand. */ + /* See rake-tasks/selenium_rake/cpp_formatter.rb instead. */ + #ifndef __IE_RETURN_TYPES_H_ + #define __IE_RETURN_TYPES_H_ + HEREDOC end def generate_file_footer diff --git a/rake_tasks/selenium_rake/crazy_fun.rb b/rake_tasks/selenium_rake/crazy_fun.rb index 1bb97f1b5f934..639f0665f8471 100644 --- a/rake_tasks/selenium_rake/crazy_fun.rb +++ b/rake_tasks/selenium_rake/crazy_fun.rb @@ -1,38 +1,28 @@ +# frozen_string_literal: true + module SeleniumRake class CrazyFun def initialize @mappings = {} - add_mapping('java_binary', SeleniumRake::DetonatingHandler.new) - add_mapping('java_library', SeleniumRake::DetonatingHandler.new) - add_mapping('java_test', SeleniumRake::DetonatingHandler.new) + add_mapping('java_binary') + add_mapping('java_library') + add_mapping('java_test') end - def add_mapping(type_name, handler) + def add_mapping(type_name, handler = detonating_handler) @mappings[type_name] = [] unless @mappings.key?(type_name) @mappings[type_name].push handler end def prebuilt_roots - @roots ||= [] + @prebuilt_roots ||= [] end def find_prebuilt(of) - of_parts = if of =~ %r{build([/\\])} - of.split(Regexp.last_match(1))[1..-1] - else - of.split(Platform.dir_separator) - end - prebuilt_roots.each do |root| root_parts = root.split('/') - - if root_parts.first == of_parts.first - of_parts[0] = root - src = of_parts.join('/') - else - src = "#{root}/#{of}" - end + src = generate_src(of, root_parts) return src if File.exist? src end @@ -45,7 +35,7 @@ def create_tasks(files) puts "Parsing #{f}" if $DEBUG outputs = BuildFile.new.parse_file(f) outputs.each do |type| - raise "No mapping for type: #{type.name}" unless @mappings.key?(type.name) + crash_if_no_mapping_key(type) mappings = @mappings[type.name] mappings.each do |mapping| @@ -54,5 +44,33 @@ def create_tasks(files) end end end + + private + + def detonating_handler + SeleniumRake::DetonatingHandler.new + end + + def generate_src(of, root_parts) + if root_parts.first == of_parts(of).first + of_parts(of)[0] = root + of_parts(of).join('/') + else + "#{root}/#{of}" + end + end + + def of_parts(of) + @of_parts ||= + if of =~ %r{build([/\\])} + of.split(Regexp.last_match(1))[1..-1] + else + of.split(Platform.dir_separator) + end + end + + def crash_if_no_mapping_key(type) + raise "No mapping for type #{type.name}" unless @mappings.key?(type.name) + end end end diff --git a/rake_tasks/selenium_rake/detonating_handler.rb b/rake_tasks/selenium_rake/detonating_handler.rb index 95a8a3848209f..a880aa4e1b3aa 100644 --- a/rake_tasks/selenium_rake/detonating_handler.rb +++ b/rake_tasks/selenium_rake/detonating_handler.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module SeleniumRake class DetonatingHandler def handle(fun, dir, args) diff --git a/rake_tasks/selenium_rake/ie_code_generator.rb b/rake_tasks/selenium_rake/ie_code_generator.rb index b528ebdef374e..ac21e78c46ea0 100644 --- a/rake_tasks/selenium_rake/ie_code_generator.rb +++ b/rake_tasks/selenium_rake/ie_code_generator.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This file auto-generates code for the IE driver. To make different language # bindings easier to maintain, it generates code for mapping numeric return # type identifiers (returned from wdGetScriptResultType) to a meaningful diff --git a/rake_tasks/selenium_rake/java_formatter.rb b/rake_tasks/selenium_rake/java_formatter.rb index 0db04d9f3456d..bb2507307a3fa 100644 --- a/rake_tasks/selenium_rake/java_formatter.rb +++ b/rake_tasks/selenium_rake/java_formatter.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This file auto-generates code for the IE driver. To make different language # bindings easier to maintain, it generates code for mapping numeric return # type identifiers (returned from wdGetScriptResultType) to a meaningful @@ -6,11 +8,12 @@ module SeleniumRake class JavaFormatter def generate_file_header - out_str = "/* AUTO GENERATED - Do not edit by hand. */\n" - out_str += "/* See rake-tasks/selenium_rake/java_formatter.rb instead. */\n" - out_str += "package org.openqa.selenium.ie;\n" - out_str += "public class IeReturnTypes {\n" - out_str + <<~HEREDOC + /* AUTO GENERATED - Do not edit by hand. */ + /* See rake-tasks/selenium_rake/java_formatter.rb instead. */ + package org.openqa.selenium.ie; + public class IeReturnTypes { + HEREDOC end def generate_file_footer diff --git a/rake_tasks/selenium_rake/type_definitions_generator.rb b/rake_tasks/selenium_rake/type_definitions_generator.rb index 3cabda9359c3c..c551666c80e25 100644 --- a/rake_tasks/selenium_rake/type_definitions_generator.rb +++ b/rake_tasks/selenium_rake/type_definitions_generator.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This file auto-generates code for the IE driver. To make different language # bindings easier to maintain, it generates code for mapping numeric return # type identifiers (returned from wdGetScriptResultType) to a meaningful @@ -26,13 +28,11 @@ def generate_defs_file(dest_file, formatter) end def generate_java_definitions(dest_file) - java_formatter = JavaFormatter.new - generate_defs_file(dest_file, java_formatter) + generate_defs_file(dest_file, JavaFormatter.new) end def generate_cpp_definitions(dest_file) - cpp_formatter = CppFormatter.new - generate_defs_file(dest_file, cpp_formatter) + generate_defs_file(dest_file, CppFormatter.new) end end end