KevinGG commented on a change in pull request #11338: [BEAM-7923] Screendiff Integration Tests URL: https://github.com/apache/beam/pull/11338#discussion_r405740866
########## File path: sdks/python/apache_beam/runners/interactive/testing/integration/screen_diff.py ########## @@ -0,0 +1,203 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on 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. +# + +"""Module to conduct screen diff based notebook integration tests.""" + +# pytype: skip-file + +from __future__ import absolute_import + +import os +import threading +import unittest +from http.server import SimpleHTTPRequestHandler +from http.server import ThreadingHTTPServer +from multiprocessing import Process + +from apache_beam.runners.interactive import interactive_environment as ie +from apache_beam.runners.interactive.testing.integration import notebook_executor + +try: + import chromedriver_binary # pylint: disable=unused-import + from needle.cases import NeedleTestCase + from needle.driver import NeedleChrome + from selenium.webdriver.chrome.options import Options + _interactive_integration_ready = ( + notebook_executor._interactive_integration_ready) +except ImportError: + _interactive_integration_ready = False + + +class ScreenDiffIntegrationTestEnvironment(object): + """A test environment to conduct screen diff integration tests for notebooks. Review comment: I've listed several problems with that approach in https://docs.google.com/document/d/1tk2P0Cu9hMjbtDWfIuQEFj07q20iqasqLPVtYVpxw3w/edit?usp=sharing. TL;DR: We can and we have such tests in place that compares the produced html outputs, but the screen diff test covers a different test scenario and should not be mutual excluded. - Many web elements are not stable across multiple runs. So we cannot simply compare the HTML as plain text between runs. - We have regex assertions in hosted notebook image release integration tests. However, regex is tedious to write, error-prone, not readable and could not cover all outputs. - There are changes we/users do not care but could cause unexpected test failure. For example, refactoring a piece of Javascript/HTML template that does not affect the final web page rendered should not cause integration test to fail. - There are changes we do care but will not cause test failure when expected. For example, when modern browser drops support to some old technology that breaks the rendering of web page even when the output HTML has not changed at all, such integration test should catch that. - Furthermore, asserting generated HTML/JavaScript/CSS is like asserting source code without executing them, thus missing runtime errors. There are situations when a malformed CSS selector could break JavaScripts, asserting notebook outputs as text will not catch it until you run the actual JavaScripts and capture the screenshot. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
