jenkins-bot has submitted this change and it was merged. Change subject: 1.0 updates ......................................................................
1.0 updates Everything works with Elasticsearch 0.90 and 1.0. Dropping support for 0.90 will remove some of the weird constructs and might speed some things up so we'll do that once we've updated WMF's cluster. This commit also makes some changes to make running the browser tests on vagrant simpler. They are bundled together because that is how I tested 1.0 and untangling them is more trouble then it is worth because we want both of them. Bug: 61879 Change-Id: I1dc24be8350b3a9d7ec9aa4fc251368c7c4b7da2 --- M .gitignore M includes/AnalysisConfigBuilder.php M includes/ElasticsearchIntermediary.php M includes/MappingConfigBuilder.php M includes/Result.php M includes/ResultsType.php M includes/Searcher.php M maintenance/updateOneSearchIndexConfig.php M tests/browser/features/step_definitions/page_steps.rb M tests/browser/features/step_definitions/search_steps.rb M tests/browser/features/support/pages/article_page.rb M tests/browser/features/update_weight.feature 12 files changed, 267 insertions(+), 93 deletions(-) Approvals: Manybubbles: Looks good to me, approved jenkins-bot: Verified diff --git a/.gitignore b/.gitignore index 1dcf7a5..7c72671 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ *~ *.kate-swp .*.swp +.indexed tests/browser/cucumber_failures.log tests/browser/.bundle tests/browser/screenshots diff --git a/includes/AnalysisConfigBuilder.php b/includes/AnalysisConfigBuilder.php index 8a0698c..f56144c 100644 --- a/includes/AnalysisConfigBuilder.php +++ b/includes/AnalysisConfigBuilder.php @@ -118,7 +118,7 @@ ), 'aggressive_splitting' => array( 'type' => 'word_delimiter', - 'stem_english_possessive' => 'false', // No need + 'stem_english_possessive' => false, // No need ), 'prefix_ngram_filter' => array( 'type' => 'edgeNGram', diff --git a/includes/ElasticsearchIntermediary.php b/includes/ElasticsearchIntermediary.php index 8802521..e7b883c 100644 --- a/includes/ElasticsearchIntermediary.php +++ b/includes/ElasticsearchIntermediary.php @@ -115,6 +115,28 @@ } /** + * Unwrap a result that we expect to be a single value. + * @param mixed $data from Elastica result + * @return mixed the single result + */ + public static function singleValue( $result, $name ) { + $data = $result->__get( $name ); + if ( $data === null ) { + return null; + } + // Elasticsearch 0.90 returns single results as, well, single results + if ( !is_array( $data ) ) { + return $data; + } + // Elasticsearch 1.0 returns them as single node arrays + $count = count( $data ); + if ( $count !== 1 ) { + wfLogWarning( "Expected a single result for $name but got $count." ); + } + return $data[ 0 ]; + } + + /** * Does this status represent an Elasticsearch parse error? * @param $status Status to check * @return boolean is this a parse error? diff --git a/includes/MappingConfigBuilder.php b/includes/MappingConfigBuilder.php index cc37dcd..cc02143 100644 --- a/includes/MappingConfigBuilder.php +++ b/includes/MappingConfigBuilder.php @@ -80,11 +80,11 @@ $config = array( 'dynamic' => false, + '_all' => array( 'enabled' => false ), 'properties' => array( 'timestamp' => array( 'type' => 'date', 'format' => 'dateOptionalTime', - 'include_in_all' => false, ), 'namespace' => $this->buildLongField(), 'namespace_text' => $this->buildKeywordField(), @@ -93,9 +93,8 @@ $this->buildStringField( 'text', $textExtraAnalyzers ), array( 'fields' => array( 'word_count' => array( 'type' => 'token_count', - 'store' => 'yes', + 'store' => true, 'analyzer' => 'plain', - 'include_in_all' => false, ) ) ) ), 'file_text' => $this->buildStringField( 'file_text', $textExtraAnalyzers ), @@ -128,26 +127,24 @@ * @return array definition of the field */ public function buildStringField( $name, $extra = array(), $enableNorms = true ) { - $norms = array( 'enabled' => $enableNorms ); + $norms = array( 'norms' => array( 'enabled' => $enableNorms ) ); + // multi_field is dead in 1.0 so we do this which actually looks less gnarly. $field = array( - 'type' => 'multi_field', + 'type' => 'string', + 'analyzer' => 'text', + 'term_vector' => 'with_positions_offsets', 'fields' => array( - $name => array( - 'type' => 'string', - 'analyzer' => 'text', - 'term_vector' => 'with_positions_offsets', - 'include_in_all' => false, - 'norms' => $norms, - ), 'plain' => array( 'type' => 'string', 'analyzer' => 'plain', 'term_vector' => 'with_positions_offsets', - 'include_in_all' => false, - 'norms' => $norms, ), ) ); + if ( !$enableNorms ) { + $field = array_merge( $field, $norms ); + $field[ 'fields' ][ 'plain' ] = array_merge( $field[ 'fields' ][ 'plain' ], $norms ); + } foreach ( $extra as $extraField ) { if ( isset( $extraField[ 'analyzer' ] ) ) { $extraName = $extraField[ 'analyzer' ]; @@ -156,9 +153,11 @@ } $field[ 'fields' ][ $extraName ] = array_merge( array( 'type' => 'string', - 'include_in_all' => false, - 'norms' => $norms, ), $extraField ); + if ( !$enableNorms ) { + $field[ 'fields' ][ $extraName ] = array_merge( + $field[ 'fields' ][ $extraName ], $norms ); + } } return $field; } @@ -171,7 +170,6 @@ return array( 'type' => 'string', 'analyzer' => 'lowercase_keyword', - 'include_in_all' => false, 'norms' => array( 'enabled' => false ), // Omit the length norm because there is only even one token 'index_options' => 'docs', // Omit the frequency and position information because neither are useful ); @@ -185,7 +183,6 @@ return array( 'type' => 'string', 'analyzer' => 'keyword', - 'include_in_all' => false, 'norms' => array( 'enabled' => false ), // Omit the length norm because there is only even one token 'index_options' => 'docs', // Omit the frequency and position information because neither are useful ); @@ -199,7 +196,6 @@ public function buildLongField( $index = true ) { $config = array( 'type' => 'long', - 'include_in_all' => false, ); if ( !$index ) { $config[ 'index' ] = 'no'; diff --git a/includes/Result.php b/includes/Result.php index f8f9227..c0f682c 100644 --- a/includes/Result.php +++ b/includes/Result.php @@ -46,7 +46,8 @@ global $wgCirrusSearchShowScore; $this->maybeSetInterwiki( $result, $interwikis ); - $this->mTitle = Title::makeTitle( $result->namespace, $result->title, '', $this->interwiki ); + $this->mTitle = Title::makeTitle( ElasticsearchIntermediary::singleValue( $result, 'namespace' ), + ElasticsearchIntermediary::singleValue( $result, 'title' ), '', $this->interwiki ); if ( $this->getTitle()->getNamespace() == NS_FILE ) { $this->mImage = wfFindFile( $this->mTitle ); } @@ -54,8 +55,9 @@ $data = $result->getData(); // Not all results requested a word count. Just pretend we have none if so $this->wordCount = isset( $data['text.word_count'] ) ? $data['text.word_count'] : 0; - $this->byteSize = $result->text_bytes; - $this->timestamp = new MWTimestamp( $result->timestamp ); + $this->byteSize = ElasticsearchIntermediary::singleValue( $result, 'text_bytes' ); + $this->timestamp = ElasticsearchIntermediary::singleValue( $result, 'timestamp' ); + $this->timestamp = new MWTimestamp( $this->timestamp ); $highlights = $result->getHighlights(); // TODO remove when Elasticsearch issue 3757 is fixed $highlights = $this->swapInPlainHighlighting( $highlights, 'redirect.title' ); @@ -72,7 +74,24 @@ } if ( !isset( $highlights[ 'title' ] ) && isset( $highlights[ 'redirect.title' ] ) ) { // Make sure to find the redirect title before escaping because escaping breaks it.... - $this->redirectTitle = $this->findRedirectTitle( $highlights[ 'redirect.title' ][ 0 ], $result->redirect ); + + // This odd juggling is the second half of the script fields hack to get redirect loaded. + // It'll go away when we switch to source filtering. + $redirects = $result->redirect; + wfDebugLog( 'CirrusSearch', "ASDFASDF " . implode( ' ', explode( "\n", var_export( $redirects, true ) ) ) ); + if ( $redirects !== null ) { + // I not null it'll always be an array. + // In Elasticsearch 0.90 it'll be an array of arrays which is what we need. + // In Elasticsearch 1.0 it'll be an array of arrays of arrays where the outer most array + // has only a single element which is exactly what would come back from 0.90. + if ( count( $redirects ) !== 0 && !array_key_exists( 'title', $redirects[ 0 ] ) ) { + wfDebugLog( 'CirrusSearch', "1.0"); + // Since the first entry doesn't have a title we assume we're in 1.0 + $redirects = $redirects[ 0 ]; + wfDebugLog( 'CirrusSearch', "ASDFASDF " . implode( ' ', explode( "\n", var_export( $redirects, true ) ) ) ); + } + } + $this->redirectTitle = $this->findRedirectTitle( $highlights[ 'redirect.title' ][ 0 ], $redirects ); $this->redirectSnipppet = $this->escapeHighlightedText( $highlights[ 'redirect.title' ][ 0 ] ); } else { $this->redirectSnipppet = ''; diff --git a/includes/ResultsType.php b/includes/ResultsType.php index 4cea699..01486bd 100644 --- a/includes/ResultsType.php +++ b/includes/ResultsType.php @@ -23,6 +23,7 @@ */ interface ResultsType { function getFields(); + function getScriptFields(); function getHighlightingConfiguration(); function transformElasticsearchResult( $suggestPrefixes, $suggestSuffixes, $result, $searchContainedSyntax ); @@ -45,6 +46,10 @@ public function getFields() { return array( 'namespace', 'title' ); + } + + public function getScriptFields() { + return null; } public function getHighlightingConfiguration() { @@ -75,7 +80,8 @@ $result, $searchContainedSyntax ) { $results = array(); foreach( $result->getResults() as $r ) { - $title = Title::makeTitle( $r->namespace, $r->title ); + $title = Title::makeTitle( ElasticsearchIntermediary::singleValue( $r, 'namespace' ), + ElasticsearchIntermediary::singleValue( $r, 'title' ) ); $highlights = $r->getHighlights(); // Now we have to use the highlights to figure out whether it was the title or the redirect @@ -92,7 +98,7 @@ // Instead of getting the redirect's real namespace we're going to just use the namespace // of the title. This is not great but OK given that we can't find cross namespace // redirects properly any way. - $title = Title::makeTitle( $r->namespace, $redirectTitle ); + $title = Title::makeTitle( ElasticsearchIntermediary::singleValue( $r, 'namespace' ), $redirectTitle ); } else if ( !isset( $highlights[ "title.$this->matchedAnalyzer" ] ) ) { // We're not really sure where the match came from so lets just pretend it was the title? wfDebugLog( 'CirrusSearch', "Title search result type hit a match but we can't " . @@ -109,8 +115,16 @@ class FullTextResultsType implements ResultsType { public function getFields() { - return array( 'id', 'title', 'namespace', 'redirect', 'timestamp', - 'text_bytes', 'text.word_count' ); + return array( 'id', 'title', 'namespace', + // 'redirect', was moved to a script field.... + 'timestamp', 'text_bytes', 'text.word_count' ); + } + + public function getScriptFields() { + // Works for both 1.0 and 0.90. + // TODO remove this in favor of source filtering when we remove support for 0.90. + // see http://www.elasticsearch.org/guide/en/elasticsearch/reference/1.x/search-request-source-filtering.html + return array( 'redirect' => new \Elastica\Script( '_source[ "redirect" ]' ) ); } /** @@ -200,4 +214,8 @@ public function getFields() { return array( 'namespace', 'namespace_text', 'title' ); } + + public function getScriptFields() { + return null; + } } diff --git a/includes/Searcher.php b/includes/Searcher.php index 95f6b3f..97cfe5f 100644 --- a/includes/Searcher.php +++ b/includes/Searcher.php @@ -384,10 +384,15 @@ $searchContainedSyntax = true; return ''; case 'intitle': - $filterDestination[] = new \Elastica\Filter\Query( new \Elastica\Query\Field( 'title', + $query = new \Elastica\Query\QueryString( $searcher->fixupWholeQueryString( $searcher->fixupQueryStringPart( $value ) - ) ) ); + ) ); + $query->setFields( array( 'title' ) ); + $query->setDefaultOperator( 'AND' ); + $query->setAllowLeadingWildcard( false ); + $query->setFuzzyPrefixLength( 2 ); + $filterDestination[] = new \Elastica\Filter\Query( $query ); $searchContainedSyntax = true; return $keepText ? "$value " : ''; default: @@ -683,6 +688,10 @@ $query = new Elastica\Query(); $query->setFields( $this->resultsType->getFields() ); + $scriptFields = $this->resultsType->getScriptFields(); + if ( $scriptFields ) { + $query->setScriptFields( $scriptFields ); + } $extraIndexes = array(); if ( $this->namespaces ) { @@ -862,6 +871,11 @@ array( 'field' => $field, 'suggest_mode' => 'always', // Forces us to generate lots of phrases to try. + // If a term appears in more then half the docs then don't try to correct it. This really + // shouldn't kick in much because we're not looking for misspellings. We're looking for phrases + // that can be might off. Like "noble prize" -> "nobel prize". In any case, the default was + // 0.01 which way too frequently decided not to correct some terms. + 'max_term_freq' => 0.5, ), ), 'highlight' => array( diff --git a/maintenance/updateOneSearchIndexConfig.php b/maintenance/updateOneSearchIndexConfig.php index 0d34258..3bad9fa 100644 --- a/maintenance/updateOneSearchIndexConfig.php +++ b/maintenance/updateOneSearchIndexConfig.php @@ -88,10 +88,20 @@ private $phraseUseText; /** + * @var bool print config as it is being checked + */ + private $printDebugCheckConfig; + + /** * @var float how much can the reindexed copy of an index is allowed to deviate from the current * copy without triggering a reindex failure */ private $reindexAcceptableCountDeviation; + + /** + * @var bool is this Elasticsearch 0.90.x? + */ + private $elasticsearch90; public function __construct() { parent::__construct(); @@ -141,6 +151,8 @@ 'rather than failing the whole reindex process. Defaults to 5.', false, true ); $maintenance->addOption( 'baseName', 'What basename to use for all indexes, ' . 'defaults to wiki id', false, true ); + $maintenance->addOption( 'debugCheckConfig', 'Print the configuration as it is checked ' . + 'to help debug unexepcted configuration missmatches.' ); } public function execute() { @@ -164,6 +176,7 @@ $this->getOption( 'reindexAcceptableCountDeviation', '5%' ) ); $this->reindexChunkSize = $this->getOption( 'reindexChunkSize', 100 ); $this->reindexRetryAttempts = $this->getOption( 'reindexRetryAttempts', 5 ); + $this->printDebugCheckConfig = $this->getOption( 'debugCheckConfig', false ); $this->langCode = $wgLanguageCode; $this->aggressiveSplitting = $wgCirrusSearchUseAggressiveSplitting; $this->prefixSearchStartsWithAny = $wgCirrusSearchPrefixSearchStartsWithAnyWord; @@ -175,6 +188,8 @@ $this->error( 'indexType option must be one of ' . implode( ', ', $indexTypes ), 1 ); } + + $this->fetchElasticsearchVersion(); if ( $this->getOption( 'forceOpen', false ) ) { $this->getIndex()->open(); @@ -201,6 +216,15 @@ "Message: $message\n" . "Trace:\n" . $trace, 1 ); } + } + + private function fetchElasticsearchVersion() { + $this->output( $this->indent . 'Fetching Elasticsearch version...' ); + $result = Connection::getClient()->request( '' ); + $result = $result->getData(); + $result = $result[ 'version' ][ 'number' ]; + $this->elasticsearch90 = strpos( $result, '0.90.' ) !== false; + $this->output( ( $this->elasticsearch90 ? '' : 'after ' ) . "0.90\n" ); } private function updateVersions() { @@ -230,9 +254,8 @@ private function validateIndexSettings() { $this->output( $this->indent . "\tValidating number of shards..." ); - $settingsObject = $this->getIndex()->getSettings(); - $settings = $settingsObject->get(); - $actualShardCount = $settings[ 'index.number_of_shards' ]; + $settings = $this->getSettings(); + $actualShardCount = $settings[ 'index' ][ 'number_of_shards' ]; if ( $actualShardCount == $this->getShardCount() ) { $this->output( "ok\n" ); } else { @@ -245,23 +268,22 @@ } $this->output( $this->indent . "\tValidating number of replicas..." ); - $actualReplicaCount = $settings[ 'index.number_of_replicas' ]; + $actualReplicaCount = $settings[ 'index' ][ 'number_of_replicas' ]; if ( $actualReplicaCount == $this->getReplicaCount() ) { $this->output( "ok\n" ); } else { $this->output( "is $actualReplicaCount but should be " . $this->getReplicaCount() . '...' ); - $settingsObject->setNumberOfReplicas( $this->getReplicaCount() ); + $this->getIndex()->getSettings()->setNumberOfReplicas( $this->getReplicaCount() ); $this->output( "corrected\n" ); } } private function validateAnalyzers() { $this->output( $this->indent . "Validating analyzers..." ); - $settingsObject = $this->getIndex()->getSettings(); - $settings = $settingsObject->get(); + $settings = $this->getSettings(); $analysisConfig = new AnalysisConfigBuilder( $this->langCode, $this->aggressiveSplitting ); $requiredAnalyzers = $analysisConfig->buildConfig(); - if ( $this->vaActualMatchRequired( 'index.analysis', $settings, $requiredAnalyzers ) ) { + if ( $this->checkConfig( $settings[ 'index' ][ 'analysis' ], $requiredAnalyzers ) ) { $this->output( "ok\n" ); } else { $this->output( "different..." ); @@ -282,42 +304,37 @@ } /** - * @param $prefix - * @param $settings - * @param $required array - * @return bool + * Load the settings array. You can't use this to set the settings, use $this->getIndex()->getSettings() for that. + * @return array of settings always in Elasticsearch 1.0 format */ - private function vaActualMatchRequired( $prefix, $settings, $required ) { - foreach( $required as $key => $value ) { - $settingsKey = $prefix . '.' . $key; - if ( is_array( $value ) ) { - if ( !$this->vaActualMatchRequired( $settingsKey, $settings, $value ) ) { - return false; - } - continue; - } - // Note that I really mean !=, not !==. Coercion is cool here. - if ( !array_key_exists( $settingsKey, $settings ) || $settings[ $settingsKey ] != $value ) { - return false; - } + private function getSettings() { + $settings = $this->getIndex()->getSettings()->get(); + if ( !$this->elasticsearch90 ) { // 1.0 compat + // Looks like we're already in the new format already + return $settings; } - return true; + $to = array(); + foreach ( $settings as $key => $value ) { + $current = &$to; + foreach ( explode( '.', $key ) as $segment ) { + $current = &$current[ $segment ]; + } + $current = $value; + } + return $to; } private function validateMapping() { - $this->output( $this->indent . "Validating mappings...\n" ); - $actualMappings = $this->getIndex()->getMapping(); - $actualMappings = $actualMappings[ $this->getIndex()->getName() ]; - - $this->output( $this->indent . "\tValidating mapping for page type..." ); + $this->output( $this->indent . "Validating mappings..." ); $requiredPageMappings = new MappingConfigBuilder( $this->prefixSearchStartsWithAny, $this->phraseUseText ); $requiredPageMappings = $requiredPageMappings->buildConfig(); - if ( array_key_exists( 'page', $actualMappings ) && - $this->vmActualMatchRequired( $actualMappings[ 'page' ], $requiredPageMappings ) ) { - $this->output( "ok\n" ); - } else { - $this->output( "different..." ); + + if ( $this->elasticsearch90 ) { + $requiredPageMappings = $this->transformMappingToElasticsearch90( $requiredPageMappings ); + } + + if ( !$this->checkMapping( $requiredPageMappings ) ) { // TODO Conflict resolution here might leave old portions of mappings $action = new \Elastica\Type\Mapping( $this->getPageType() ); foreach ( $requiredPageMappings as $key => $value ) { @@ -334,37 +351,123 @@ } } + private function transformMappingToElasticsearch90( $mapping ) { + foreach ( $mapping[ 'properties' ] as $name => $field ) { + if ( array_key_exists( 'fields', $field ) ) { + // Move the main field configuration to a subfield + $field[ 'fields' ][ $name ] = $field; + unset( $field[ 'fields' ][ $name ][ 'fields' ] ); + // Now clear everything from the "main" field that we've moved + $mapping[ 'properties' ][ $name ] = array( + 'type' => 'multi_field', + 'fields' => $field[ 'fields' ], + ); + } + if ( array_key_exists( 'properties', $field ) ) { + $mapping[ 'properties' ][ $name ] = $this->transformMappingToElasticsearch90( + $field ); + } + } + return $mapping; + } + + /** + * Check that the mapping returned from Elasticsearch is as we want it. + * @param array $requiredPageMappings the mappings we want + * @return bool is the mapping good enough for us? + */ + private function checkMapping( $requiredPageMappings ) { + $indexName = $this->getIndex()->getName(); + $actualMappings = $this->getIndex()->getMapping(); + if ( !array_key_exists( $indexName, $actualMappings ) ) { + $this->output( "nonexistent..."); + return false; + } + $actualMappings = $actualMappings[ $indexName ]; + if ( !$this->elasticsearch90 ) { // 1.0 compat + $actualMappings = $actualMappings[ 'mappings' ]; + } + + $this->output( "\n" . $this->indent . "\tValidating mapping for page type..." ); + if ( array_key_exists( 'page', $actualMappings ) && + $this->checkConfig( $actualMappings[ 'page' ], $requiredPageMappings ) ) { + $this->output( "ok\n" ); + return true; + } else { + $this->output( "different..." ); + return false; + } + } + /** * @param $actual * @param $required array * @return bool */ - private function vmActualMatchRequired( $actual, $required ) { + private function checkConfig( $actual, $required, $indent = null ) { + if ( $indent === null ) { + $indent = $this->indent . "\t\t"; + } foreach( $required as $key => $value ) { + $this->debugCheckConfig( "\n$indent$key: " ); if ( !array_key_exists( $key, $actual ) ) { + $this->debugCheckConfig( "not found..." ); + if ( $key === '_all' ) { + // The _all field never comes back so we just have to assume it + // is set correctly. + $this->debugCheckConfig( "was the all field so skipping..." ); + continue; + } return false; } if ( is_array( $value ) ) { + $this->debugCheckConfig( "descend..." ); if ( !is_array( $actual[ $key ] ) ) { + $this->debugCheckConfig( "other not array..." ); return false; } - if ( !$this->vmActualMatchRequired( $actual[ $key ], $value ) ) { + if ( !$this->checkConfig( $actual[ $key ], $value, $indent . "\t" ) ) { return false; } continue; } - if ( $actual[ $key ] === 'false' ) { - $actual[ $key ] = false; - } + $actual[ $key ] = $this->normalizeConfigValue( $actual[ $key ] ); + $value = $this->normalizeConfigValue( $value ); + $this->debugCheckConfig( $actual[ $key ] . " ?? $value..." ); // Note that I really mean !=, not !==. Coercion is cool here. + // print $actual[ $key ] . " $value\n"; if ( $actual[ $key ] != $value ) { + $this->debugCheckConfig( 'different...' ); return false; } } return true; } + /** + * Normalize a config value for comparison. Elasticsearch will accept all kinds + * of config values but it tends to through back 'true' for true and 'false' for + * false so we normalize everything. Sometimes, oddly, it'll through back false + * for false.... + * @param mixed $value config value + * @return mixes value normalized + */ + private function normalizeConfigValue( $value ) { + if ( $value === true ) { + return 'true'; + } else if ( $value === false ) { + return 'false'; + } + return $value; + } + + private function debugCheckConfig( $string ) { + if ( $this->printDebugCheckConfig ) { + $this->output( $string ); + } + } + private function validateAlias() { $this->output( $this->indent . "Validating aliases...\n" ); // Since validate the specific alias first as that can cause reindexing diff --git a/tests/browser/features/step_definitions/page_steps.rb b/tests/browser/features/step_definitions/page_steps.rb index 2a31d61..ff3cc64 100644 --- a/tests/browser/features/step_definitions/page_steps.rb +++ b/tests/browser/features/step_definitions/page_steps.rb @@ -80,20 +80,16 @@ md5 = Digest::MD5.hexdigest(File.read(contents)) md5_string = "md5: #{md5}" visit(ArticlePage, using_params: {page_name: title}) do |page| - if page.file_history? && page.file_last_comment? && page.file_last_comment.include?(md5_string) + if page.file_history? && + page.file_last_comment? && + page.file_last_comment.include?(md5_string) return end - if !(page.upload_new_version? || page.upload?) + if !page.upload? step "I am logged in" - visit(ArticlePage, using_params: {page_name: title}) end - if page.upload? - # New file, upload it - page.upload - else - # Existing file, update it - page.upload_new_version - end + # If this doesn't exist then file uploading is probably disabled + page.upload end on(UploadFilePage) do |page| page.description = description + "\n" + md5_string diff --git a/tests/browser/features/step_definitions/search_steps.rb b/tests/browser/features/step_definitions/search_steps.rb index b121259..2362e77 100644 --- a/tests/browser/features/step_definitions/search_steps.rb +++ b/tests/browser/features/step_definitions/search_steps.rb @@ -37,10 +37,16 @@ if page.simple_search_button? then page.simple_search_button else - #Since there isn't a simple search button on this page we're going to have - #to use the "containing..." drop down.... - on(SearchPage).search_special_element.when_present.should exist - on(SearchPage).search_special_element.click + #Since there isn't a search button on this page we're going to have + #to use the "containing..." drop down.... We can't even click the + #search_button because that is a "go" search and we need one that won't + #drop us directly to the page on a perfect match + + #I have no idea why, but just clicking on the element isn't good enough + #so we deploy this hack copied from mediawiki.searchSuggest.js + page.execute_script("$( '\#searchInput' ).closest( 'form' ) + .append( $( '<input type=\"hidden\" name=\"fulltext\" value=\"1\"/>' ) );") + page.search_button end end end diff --git a/tests/browser/features/support/pages/article_page.rb b/tests/browser/features/support/pages/article_page.rb index ddee828..5d26e9c 100644 --- a/tests/browser/features/support/pages/article_page.rb +++ b/tests/browser/features/support/pages/article_page.rb @@ -5,9 +5,8 @@ h1(:title, id: "firstHeading") table(:file_history, :class => "filehistory") - cell(:file_last_comment){ table_element(:class => "filehistory")[1][5] } - link(:upload, text: "upload it") - link(:upload_new_version, text: "Upload a new version of this file") + cell(:file_last_comment){ |page| page.file_history_element[1][page.file_history_element[1].columns - 1] } + link(:upload, text: "Upload file") link(:create_link, text: "Create") link(:create_source_link, text: "Create source") end diff --git a/tests/browser/features/update_weight.feature b/tests/browser/features/update_weight.feature index ea4a0f4..5054e47 100644 --- a/tests/browser/features/update_weight.feature +++ b/tests/browser/features/update_weight.feature @@ -12,7 +12,7 @@ Then within 10 seconds searching for WeightedLink%{epoch} yields WeightedLink%{epoch} 2 as the first result When a page named WeightedLink%{epoch} 1/1 exists with contents [[WeightedLink%{epoch} 1]] And a page named WeightedLink%{epoch} 1/2 exists with contents [[WeightedLink%{epoch} 1]] - Then within 10 seconds searching for WeightedLink%{epoch} yields WeightedLink%{epoch} 1 as the first result + Then within 20 seconds searching for WeightedLink%{epoch} yields WeightedLink%{epoch} 1 as the first result Scenario: Pages weights are updated when links are removed from them Given a page named WeightedLinkRemoveUpdate%{epoch} 1/1 exists with contents [[WeightedLinkRemoveUpdate%{epoch} 1]] @@ -23,7 +23,7 @@ Then within 10 seconds searching for WeightedLinkRemoveUpdate%{epoch} yields WeightedLinkRemoveUpdate%{epoch} 1 as the first result When a page named WeightedLinkRemoveUpdate%{epoch} 1/1 exists with contents [[Junk]] And a page named WeightedLinkRemoveUpdate%{epoch} 1/2 exists with contents [[Junk]] - Then within 10 seconds searching for WeightedLinkRemoveUpdate%{epoch} yields WeightedLinkRemoveUpdate%{epoch} 2 as the first result + Then within 20 seconds searching for WeightedLinkRemoveUpdate%{epoch} yields WeightedLinkRemoveUpdate%{epoch} 2 as the first result Scenario: Pages weights are updated when new pages link to their redirects Given a page named WeightedLinkRdir%{epoch} 1/Redirect exists with contents #REDIRECT [[WeightedLinkRdir%{epoch} 1]] @@ -34,7 +34,7 @@ Then within 10 seconds searching for WeightedLinkRdir%{epoch} yields WeightedLinkRdir%{epoch} 2 as the first result When a page named WeightedLinkRdir%{epoch} 1/1 exists with contents [[WeightedLinkRdir%{epoch} 1/Redirect]] And a page named WeightedLinkRdir%{epoch} 1/2 exists with contents [[WeightedLinkRdir%{epoch} 1/Redirect]] - Then within 10 seconds searching for WeightedLinkRdir%{epoch} yields WeightedLinkRdir%{epoch} 1 as the first result + Then within 20 seconds searching for WeightedLinkRdir%{epoch} yields WeightedLinkRdir%{epoch} 1 as the first result Scenario: Pages weights are updated when links are removed from their redirects Given a page named WLRURdir%{epoch} 1/1 exists with contents [[WLRURdir%{epoch} 1/Redirect]] @@ -47,7 +47,7 @@ Then within 10 seconds searching for WLRURdir%{epoch} yields WLRURdir%{epoch} 1 as the first result When a page named WLRURdir%{epoch} 1/1 exists with contents [[Junk]] And a page named WLRURdir%{epoch} 1/2 exists with contents [[Junk]] - Then within 10 seconds searching for WLRURdir%{epoch} yields WLRURdir%{epoch} 2 as the first result + Then within 20 seconds searching for WLRURdir%{epoch} yields WLRURdir%{epoch} 2 as the first result Scenario: Redirects to redirects don't count in the score Given a page named WLDoubleRdir%{epoch} 1/Redirect exists with contents #REDIRECT [[WLDoubleRdir%{epoch} 1]] @@ -59,4 +59,4 @@ And a page named WLDoubleRdir%{epoch} 2/1 exists with contents [[WLDoubleRdir%{epoch} 2/Redirect]] And a page named WLDoubleRdir%{epoch} 2/2 exists with contents [[WLDoubleRdir%{epoch} 2/Redirect]] And a page named WLDoubleRdir%{epoch} 2 exists - When within 10 seconds searching for WLDoubleRdir%{epoch} yields WLDoubleRdir%{epoch} 2 as the first result + When within 20 seconds searching for WLDoubleRdir%{epoch} yields WLDoubleRdir%{epoch} 2 as the first result -- To view, visit https://gerrit.wikimedia.org/r/115215 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1dc24be8350b3a9d7ec9aa4fc251368c7c4b7da2 Gerrit-PatchSet: 8 Gerrit-Project: mediawiki/extensions/CirrusSearch Gerrit-Branch: master Gerrit-Owner: Manybubbles <never...@wikimedia.org> Gerrit-Reviewer: Chad <ch...@wikimedia.org> Gerrit-Reviewer: Hashar <has...@free.fr> Gerrit-Reviewer: Manybubbles <never...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits