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.

Sign up for GitHub

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

Jump to bottom

Test installer #57

Merged
merged 10 commits into from
Aug 3, 2019
Merged

Test installer #57

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5625a4b
added: tests for apertium/installer.py
singh-lokendra Jul 31, 2019
98d1678
update: wrapper calls
singh-lokendra Jul 31, 2019
8dc6f91
update: build-swig-wrapper
singh-lokendra Aug 1, 2019
817cbdd
utils.py: moved dictionary_path to if block
singh-lokendra Aug 1, 2019
8993514
tests: use inbuilt methods for assertion
singh-lokendra Aug 1, 2019
76a8664
update class name in apertium_core
singh-lokendra Aug 1, 2019
1327f64
clone apertium master, branch merged
singh-lokendra Aug 1, 2019
fc602b5
test: check all the binaries used for subprocess call
singh-lokendra Aug 1, 2019
529b811
test apertium binaries availability in single test
singh-lokendra Aug 2, 2019
0b7a85b
added: comment in install_apertium() test
singh-lokendra Aug 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions apertium/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,34 +56,32 @@ def execute_pipeline(inp: str, commands: List[List[str]]) -> str:
input_file_name, output_file_name = input_file.name, output_file.name

arg = command[1][1] if len(command) >= 3 else ''
path = command[-1]
dictionary_path = command[-1]
singh-lokendra marked this conversation as resolved.
Show resolved Hide resolved
text = end.decode()
input_file.write(text)
input_file.close()

if 'lt-proc' == command[0]:
lttoolbox.LtLocale.tryToSetLocale()
fst = lttoolbox.FST()
fst = lttoolbox.FST(dictionary_path)
if not fst.valid():
raise ValueError('FST Invalid')
fst = lttoolbox.FST()
fst.lt_proc(arg, path, input_file_name, output_file_name)
fst.lt_proc(arg, input_file_name, output_file_name)
elif 'lrx-proc' == command[0]:
lextools.LtLocale.tryToSetLocale()
lrx = lextools.LRX()
lrx.lrx_proc(arg, path, input_file.name, output_file.name)
lrx = lextools.LRXProc(dictionary_path)
lrx.lrx_proc(arg, input_file.name, output_file.name)
elif 'apertium-transfer' == command[0]:
obj = apertium_core.apertium()
obj.transfer_text(arg, command[2], command[3], input_file.name, output_file.name)
obj = apertium_core.transfer(command[2], command[3])
obj.transfer_text(arg, input_file.name, output_file.name)
elif 'apertium-interchunk' == command[0]:
obj = apertium_core.apertium()
obj.interchunk_text(arg, command[1], command[2], input_file.name, output_file.name)
obj = apertium_core.interchunk(command[1], command[2])
obj.interchunk_text(arg, input_file.name, output_file.name)
elif 'apertium-postchunk' == command[0]:
obj = apertium_core.apertium()
obj.postchunk_text(arg, command[1], command[2], input_file.name, output_file.name)
obj = apertium_core.postchunk(command[1], command[2])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to start off but we really need to cache all these to get any benefit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about functools.lru_cache, available in python https://docs.python.org/3/library/functools.html?highlight=lru_cache#functools.lru_cache

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean caching the result of postchunk/interchunk/etc. I mean keeping the objects around to avoid initialization costs.

obj.postchunk_text(arg, input_file.name, output_file.name)
elif 'apertium-pretransfer' == command[0]:
obj = apertium_core.apertium()
obj.pretransfer(arg, input_file.name, output_file.name)
apertium_core.pretransfer(arg, input_file.name, output_file.name)
elif 'apertium-tagger' == command[0]:
command += [input_file.name, output_file.name]
apertium_core.tagger(len(command), command)
Expand Down
10 changes: 9 additions & 1 deletion build-swig-wrapper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,18 @@ make -j2
python3 setup.py install
popd

git clone --depth 1 https://github.com/apertium/apertium.git apertium-core
git clone --depth 1 -b wrapper_optimise https://github.com/apertium/apertium.git apertium-core
pushd apertium-core
./autogen.sh --enable-python-bindings
cd python
make -j2
python3 setup.py install
popd

git clone --depth 1 https://github.com/apertium/lttoolbox.git
pushd lttoolbox
./autogen.sh --enable-python-bindings
cd python
make -j2
python3 setup.py install
popd
30 changes: 30 additions & 0 deletions tests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import importlib.util
import os
import platform
import shutil
import sys
import unittest

Expand Down Expand Up @@ -69,6 +72,33 @@ def test_uninstalled_mode(self):
apertium.generate('spa', 'cat<n><pl>')


class TestInstallation(unittest.TestCase):
def test_apertium_base(self):
apertium.installer.install_apertium()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should probably ensure that lt-proc doesn't already exist. It should also test for all the binaries, not just lt-proc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should probably ensure that lt-proc doesn't already exist.

Can you explain a bit more

It should also test for all the binaries, not just lt-proc?

Since the installer is installing apertium-all-dev, I am assuming all of the binaries to be available in the packages. I am checking existence of lt-proc as its the most used binary

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer all the ones we use to be tested. Tests should assume as little as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I've over ridden the constructor, installation process is repeated 8 times while running tests https://travis-ci.com/apertium/apertium-python/jobs/221738455#L1714. What's the reason behind this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure but you don't need to make each assert its own test. They can all be one test where you loop through the binaries you expect. Also, I would check that they don't exist before installation. Otherwise, you can't be certain your installation actually did anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The binaries would be available after running the installer. Should I create an uninstaller as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I don't think there's a good way to test this particular aspect then. Or at least I can't think of one atm. I don't think it's worth the effort to create an uninstaller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment that notes that this test doesn't actually test as much as it might seem to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current name is misleading. Should I rename it to test_apertium_binaries

apertium_exists = False
if shutil.which('lt-proc'):
singh-lokendra marked this conversation as resolved.
Show resolved Hide resolved
apertium_exists = True
self.assertTrue(apertium_exists, 'apertium binaries not find')

def test_install_module(self):
apertium.installer.install_module('kir')
importlib.reload(apertium)
try:
apertium.analyze('kir', 'cats')
except apertium.ModeNotInstalled:
self.fail('apetium.install_module not working')
singh-lokendra marked this conversation as resolved.
Show resolved Hide resolved

def test_install_wrapper(self):
apertium.installer.install_wrapper('python3-lttoolbox')
if platform.system() == 'Linux':
sys.path.append('/usr/lib/python3/dist-packages')
spec = importlib.util.find_spec('lttoolbox')
apertium_exists = False
if spec:
singh-lokendra marked this conversation as resolved.
Show resolved Hide resolved
apertium_exists = True
self.assertTrue(apertium_exists, 'Wrapper not installed')


class TestTranslate(unittest.TestCase):
def test_translator_en_spa(self):
translator = apertium.Translator('eng', 'spa')
Expand Down