Merge branch 'issue-1673' into calamares

FIXES #1673

src/modules/netinstall/CMakeLists.txt

@@ -26,6 +26,8 @@ calamares_add_test(
     netinstalltest
     SOURCES
         Tests.cpp
+        Config.cpp
+        LoaderQueue.cpp
         PackageTreeItem.cpp
         PackageModel.cpp
     LIBRARIES

src/modules/netinstall/Config.cpp

@@ -97,7 +97,6 @@ Config::loadGroupList( const QVariantList& groupData )
     {
         setStatus( Status::Ok );
     }
-    emit statusReady();
 }
 
 void
@@ -108,6 +107,7 @@ Config::loadingDone()
         m_queue->deleteLater();
         m_queue = nullptr;
     }
+    emit statusReady();
 }
 
 
@@ -136,25 +136,23 @@ Config::setConfigurationMap( const QVariantMap& configurationMap )
     // Lastly, load the groups data
     const QString key = QStringLiteral( "groupsUrl" );
     const auto& groupsUrlVariant = configurationMap.value( key );
+    m_queue = new LoaderQueue( this );
     if ( groupsUrlVariant.type() == QVariant::String )
     {
-        m_queue = new LoaderQueue( this );
         m_queue->append( SourceItem::makeSourceItem( groupsUrlVariant.toString(), configurationMap ) );
     }
     else if ( groupsUrlVariant.type() == QVariant::List )
     {
-        m_queue = new LoaderQueue( this );
         for ( const auto& s : groupsUrlVariant.toStringList() )
         {
             m_queue->append( SourceItem::makeSourceItem( s, configurationMap ) );
         }
     }
-    if ( m_queue && m_queue->count() > 0 )
+
-    {
+    setStatus( required() ? Status::FailedNoData : Status::Ok );
-        cDebug() << "Loading netinstall from" << m_queue->count() << "alternate sources.";
+    cDebug() << "Loading netinstall from" << m_queue->count() << "alternate sources.";
-        connect( m_queue, &LoaderQueue::done, this, &Config::loadingDone );
+    connect( m_queue, &LoaderQueue::done, this, &Config::loadingDone );
-        m_queue->load();
+    m_queue->load();
-    }
 }
 
 void

src/modules/netinstall/Config.h

@@ -49,10 +49,12 @@ public:
         FailedNetworkError,
         FailedBadData,
         FailedNoData
-
     };
 
+    /// Human-readable, translated representation of the status
     QString status() const;
+    /// Internal code for the status
+    Status statusCode() const { return m_status; }
     void setStatus( Status s );
 
     bool required() const { return m_required; }

src/modules/netinstall/LoaderQueue.cpp

@@ -25,6 +25,10 @@
  * On destruction, a new call to fetchNext() is queued, so that
  * the queue continues loading. Calling release() before the
  * destructor skips the fetchNext(), ending the queue-loading.
+ *
+ * Calling done(b) is a conditional release: if @p b is @c true,
+ * queues a call to done() on the queue and releases it; otherwise,
+ * does nothing.
  */
 class FetchNextUnless
 {
@@ -41,6 +45,17 @@ public:
         }
     }
     void release() { m_q = nullptr; }
+    void done( bool b )
+    {
+        if ( b )
+        {
+            if ( m_q )
+            {
+                QMetaObject::invokeMethod( m_q, "done", Qt::QueuedConnection );
+            }
+            release();
+        }
+    }
 
 private:
     LoaderQueue* m_q = nullptr;
@@ -83,7 +98,6 @@ LoaderQueue::fetchNext()
 {
     if ( m_queue.isEmpty() )
     {
-        m_config->setStatus( Config::Status::FailedBadData );
         emit done();
         return;
     }
@@ -138,7 +152,7 @@ LoaderQueue::fetch( const QUrl& url )
 void
 LoaderQueue::dataArrived()
 {
-    FetchNextUnless finished( this );
+    FetchNextUnless next( this );
 
     if ( !m_reply || !m_reply->isFinished() )
     {
@@ -170,16 +184,14 @@ LoaderQueue::dataArrived()
 
         if ( groups.IsSequence() )
         {
-            finished.release();
             m_config->loadGroupList( CalamaresUtils::yamlSequenceToVariant( groups ) );
-            emit done();
+            next.done( m_config->statusCode() == Config::Status::Ok );
         }
         else if ( groups.IsMap() )
         {
-            finished.release();
             auto map = CalamaresUtils::yamlMapToVariant( groups );
             m_config->loadGroupList( map.value( "groups" ).toList() );
-            emit done();
+            next.done( m_config->statusCode() == Config::Status::Ok );
         }
         else
         {

src/modules/netinstall/Tests.cpp

@@ -7,13 +7,17 @@
  *
  */
 
+#include "Config.h"
 #include "PackageModel.h"
 #include "PackageTreeItem.h"
 
 #include "utils/Logger.h"
+#include "utils/NamedEnum.h"
 #include "utils/Variant.h"
 #include "utils/Yaml.h"
 
+#include <KMacroExpander>
+
 #include <QtTest/QtTest>
 
 class ItemTests : public QObject
@@ -40,6 +44,9 @@ private Q_SLOTS:
     void testCompare();
     void testModel();
     void testExampleFiles();
+
+    void testUrlFallback_data();
+    void testUrlFallback();
 };
 
 ItemTests::ItemTests() {}
@@ -326,6 +333,93 @@ ItemTests::testExampleFiles()
     }
 }
 
+void
+ItemTests::testUrlFallback_data()
+{
+    QTest::addColumn< QString >( "filename" );
+    QTest::addColumn< int >( "status" );
+    QTest::addColumn< int >( "count" );
+
+    using S = Config::Status;
+
+    QTest::newRow( "bad" ) << "1a-single-bad.conf" << smash( S::FailedBadConfiguration ) << 0;
+    QTest::newRow( "empty" ) << "1a-single-empty.conf" << smash( S::FailedNoData ) << 0;
+    QTest::newRow( "error" ) << "1a-single-error.conf" << smash( S::FailedBadData ) << 0;
+    QTest::newRow( "two" ) << "1b-single-small.conf" << smash( S::Ok ) << 2;
+    QTest::newRow( "five" ) << "1b-single-large.conf" << smash( S::Ok ) << 5;
+    QTest::newRow( "none" ) << "1c-none.conf" << smash( S::FailedNoData ) << 0;
+    QTest::newRow( "unset" ) << "1c-unset.conf" << smash( S::FailedNoData ) << 0;
+    // Finds small, then stops
+    QTest::newRow( "fallback-small" ) << "1d-fallback-small.conf" << smash( S::Ok ) << 2;
+    // Finds large, then stops
+    QTest::newRow( "fallback-large" ) << "1d-fallback-large.conf" << smash( S::Ok ) << 5;
+    // Finds empty, finds small
+    QTest::newRow( "fallback-mixed" ) << "1d-fallback-mixed.conf" << smash( S::Ok ) << 2;
+    // Finds empty, then bad
+    QTest::newRow( "fallback-bad" ) << "1d-fallback-bad.conf" << smash( S::FailedBadConfiguration ) << 0;
+}
+
+void
+ItemTests::testUrlFallback()
+{
+    Logger::setupLogLevel( Logger::LOGDEBUG );
+    QFETCH( QString, filename );
+    QFETCH( int, status );
+    QFETCH( int, count );
+
+    cDebug() << "Loading" << filename;
+
+    // BUILD_AS_TEST is the source-directory path
+    QString testdir = QString( "%1/tests" ).arg( BUILD_AS_TEST );
+    QFile fi( QString( "%1/%2" ).arg( testdir, filename ) );
+    QVERIFY( fi.exists() );
+
+    Config c;
+
+    QFile yamlFile( fi.fileName() );
+    if ( yamlFile.exists() && yamlFile.open( QFile::ReadOnly | QFile::Text ) )
+    {
+        QString ba( yamlFile.readAll() );
+        QVERIFY( ba.length() > 0 );
+        QHash< QString, QString > replace;
+        replace.insert( "TESTDIR", testdir );
+        QString correctedDocument = KMacroExpander::expandMacros( ba, replace, '$' );
+
+        try
+        {
+            YAML::Node yamldoc = YAML::Load( correctedDocument.toUtf8() );
+            auto map = CalamaresUtils::yamlToVariant( yamldoc ).toMap();
+            QVERIFY( map.count() > 0 );
+            c.setConfigurationMap( map );
+        }
+        catch ( YAML::Exception& e )
+        {
+            bool badYaml = true;
+            QVERIFY( !badYaml );
+        }
+    }
+    else
+    {
+        QCOMPARE( QStringLiteral( "not found" ), fi.fileName() );
+    }
+
+    // Each of the configs sets required to **true**, which is not the default
+    QVERIFY( c.required() );
+
+    // Now give the loader time to complete
+    QEventLoop loop;
+    connect( &c, &Config::statusReady, &loop, &QEventLoop::quit );
+    QSignalSpy spy( &c, &Config::statusReady );
+    QTimer::singleShot( std::chrono::seconds(1), &loop, &QEventLoop::quit );
+    loop.exec();
+
+    // Check it didn't time out
+    QCOMPARE( spy.count(), 1 );
+    // Check YAML-loading results
+    QCOMPARE( smash( c.statusCode() ), status );
+    QCOMPARE( c.model()->rowCount(), count );
+}
+
 
 QTEST_GUILESS_MAIN( ItemTests )
 

src/modules/netinstall/tests/1a-single-bad.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/bad.yaml

src/modules/netinstall/tests/1a-single-empty.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-empty.yaml

src/modules/netinstall/tests/1a-single-error.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-error.yaml

src/modules/netinstall/tests/1b-single-large.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-large.yaml

src/modules/netinstall/tests/1b-single-small.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-small.yaml

src/modules/netinstall/tests/1c-none.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl: []

src/modules/netinstall/tests/1c-unset.conf

@@ -0,0 +1,5 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true

src/modules/netinstall/tests/1d-fallback-bad.conf

@@ -0,0 +1,10 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-nonexistent.yaml
+    - file://$TESTDIR/data-empty.yaml
+    - file://$TESTDIR/data-empty.yaml
+    - file://$TESTDIR/data-bad.yaml

src/modules/netinstall/tests/1d-fallback-large.conf

@@ -0,0 +1,10 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-nonexistent.yaml
+    - file://$TESTDIR/data-bad.yaml
+    - file://$TESTDIR/data-large.yaml
+    - file://$TESTDIR/data-small.yaml

src/modules/netinstall/tests/1d-fallback-mixed.conf

@@ -0,0 +1,13 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-nonexistent.yaml
+    - file://$TESTDIR/data-empty.yaml
+    - file://$TESTDIR/data-bad.yaml
+    - file://$TESTDIR/data-empty.yaml
+    - file://$TESTDIR/data-small.yaml
+    - file://$TESTDIR/data-large.yaml
+    - file://$TESTDIR/data-bad.yaml

src/modules/netinstall/tests/1d-fallback-small.conf

@@ -0,0 +1,10 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+required: true
+groupsUrl:
+    - file://$TESTDIR/data-nonexistent.yaml
+    - file://$TESTDIR/data-bad.yaml
+    - file://$TESTDIR/data-small.yaml
+    - file://$TESTDIR/data-large.yaml

src/modules/netinstall/tests/data-empty.yaml

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+---
+bogus: true
+

src/modules/netinstall/tests/data-error.yaml

@@ -0,0 +1,5 @@
+derp
+derp
+herpa-derp: no
+--
+# This file is not valid YAML

src/modules/netinstall/tests/data-large.yaml

@@ -0,0 +1,38 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+- name: "Default"
+  description: "Default group"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - base
+- name: "Two"
+  description: "group 2"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - chakra-live-two
+- name: "Three"
+  description: "group 3"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - chakra-live-three
+- name: "Four"
+  description: "group 4"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - chakra-live-four
+- name: "Five"
+  description: "group 5"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - chakra-live-five

src/modules/netinstall/tests/data-small.yaml

@@ -0,0 +1,17 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+- name: "Default"
+  description: "Default group"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - base
+- name: "Second"
+  description: "Second group"
+  hidden: false
+  selected: true
+  critical: false
+  packages:
+    - chakra-live-skel